Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AAE-22900] Update ObjectDataTableAdapter to support server and clien… #10418

Conversation

amolodyh-hyland
Copy link
Contributor

@amolodyh-hyland amolodyh-hyland commented Nov 20, 2024

Fixes AAE-22900. This adds server sorting for the base ObjectDataTableAdapter to enable tables in tabs such as "Process Admin" User Tasks, Services Tasks, and Applications List.

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation
  • Other... Please describe:

What is the current behaviour? (You can also link to an open issue here)
AAE-22900

What is the new behaviour?

Allows for enabling server sorting where it the adapter does not sort the data. The server data is rendered in the order it comes in.

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

This a partial fix to the provided story AAE-22900. After this is merged, hxp-frontend-apps will need to be updated by updating the applications-list-filtered-data-adapter adapter as well.

@CLAassistant
Copy link

CLAassistant commented Nov 20, 2024

CLA assistant check
All committers have signed the CLA.

@@ -21,7 +21,7 @@ import { ProcessInstanceCloudListViewModel } from '../models/perocess-instance-c

export class ProcessListDatatableAdapter extends ObjectDataTableAdapter {
constructor(data: ProcessInstanceCloudListViewModel[], schema: DataColumn<ProcessListDataColumnCustomData>[]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DenysVuika @eromano Do we want to provide the ability to set the sortingMode in these extending classes so we can utilize feature flags in the hxp-frontend-apps projects? Not sure if we want a feature flag for this but if we did, we'd need to be able to provide the sort option here.

Based on the information I've received, we don't want pages using these adapters to use client sorting so the constructor parameter would be redundant. Also, we would have to remove the constructor parameter once the feature flag is removed so it seems architecturally redundant.

@@ -21,7 +21,7 @@ import { TaskInstanceCloudListViewModel } from '../models/task-cloud-view.model'

export class TasksListDatatableAdapter extends ObjectDataTableAdapter {
constructor(data: TaskInstanceCloudListViewModel[], schema: DataColumn<ProcessListDataColumnCustomData>[]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DenysVuika @eromano Same question for this as the other adapter class comment.

this._rows = [];
this._columns = [];
this._sortingMode = sortingMode?.toString().toLowerCase() === 'server' ? 'server' : 'client';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: toString() probably isn't necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just trying to be super careful incase sortingMode is not provided as a string. You think that's overkill?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really hurt anything, but I would expect all consumers of ObjectDataTableAdapter to be using TypeScript typings, and not need the additional javascript safety checks. I spot checked some files and didn't see anywhere else where typings weren't trusted, but I could have easily missed another file that does.

Personally, I would just account for sortingMode being undefined, but if it is defined assume its a SortingMode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, makes sense to me. I'll update it.

Copy link

@sdonnell-hyland sdonnell-hyland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@amolodyh-hyland amolodyh-hyland force-pushed the bug/AAE-22900-server-sorting branch from 9361b61 to d3353f4 Compare November 22, 2024 16:40
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants