Skip to content

feat(IgxGridBaseDirective): Add Single and Multiple sorting options #10336

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

Merged
merged 49 commits into from
Apr 27, 2022

Conversation

katherinedragieva
Copy link
Contributor

@katherinedragieva katherinedragieva commented Oct 21, 2021

Closes #9674
Closes #11269
Closes #10765

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

Copy link
Contributor

@hanastasov hanastasov left a comment

Choose a reason for hiding this comment

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

Good job.
Address the comments and fix the lint errors.

@@ -4450,6 +4478,12 @@ export abstract class IgxGridBaseDirective extends DisplayDensityBase implements
} else {
if (expression.dir === SortingDirection.None) {
this.gridAPI.remove_grouping_expression(expression.fieldName);
} else if (this._sortingOptions.mode === 'single') {
Copy link
Member

@rkaraivanov rkaraivanov Oct 21, 2021

Choose a reason for hiding this comment

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

My idea was to move this logic over here:

public sort(expression: ISortingExpression): void {
if (expression.dir === SortingDirection.None) {
this.remove_grouping_expression(expression.fieldName);
}
const sortingState = cloneArray(this.grid.sortingExpressions);
this.prepare_sorting_expression([sortingState], expression);
this.grid.sortingExpressions = sortingState;

However now that I look at it, I don't think we have cleared the interaction between single mode sorting
and the group by feature of the grid.

@hanastasov @ChronosSF Maybe we should start discussing this before moving forward with this PR

@hanastasov hanastasov added 🛠️ status: in-development Issues and PRs with active development on them and removed ❌ status: awaiting-test PRs awaiting manual verification labels Oct 22, 2021
@github-actions
Copy link

There has been no recent activity and this PR has been marked inactive.

@github-actions github-actions bot added the status: inactive Used to stale issues and pull requests label Jan 24, 2022
@hanastasov hanastasov removed the status: inactive Used to stale issues and pull requests label Jan 25, 2022
@ChronosSF ChronosSF changed the base branch from master to 13.1.x March 2, 2022 10:31
@ddincheva ddincheva changed the base branch from 13.1.x to master April 12, 2022 06:41
@ddincheva ddincheva added ❌ status: awaiting-test PRs awaiting manual verification and removed 🛠️ status: in-development Issues and PRs with active development on them labels Apr 12, 2022
@ddincheva ddincheva requested a review from rkaraivanov April 14, 2022 13:02
@hanastasov hanastasov self-requested a review April 19, 2022 11:09
@hanastasov hanastasov added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels Apr 21, 2022
Copy link
Contributor

@hanastasov hanastasov left a comment

Choose a reason for hiding this comment

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

Generally the code looks OK to me and according to expectation set from the spec. I found the following bugs we need to address:

  • Sort indication is not refreshed
  1. sort by multiple columns
  2. Set sorting mode to single
  3. result: column headers still have the sort css classes
  • clearSort and sort methods do not refresh the grid
  1. sort columns
  2. grid.clearSort() does not refresh the grid state

OR

  1. sort columns via API
  2. grid is not refreshed
  • Sorting via API changes the grouping expressions
  1. group by column ContactTitle in ASC order
  2. sort programatically the ContactTitle column by DESC order:
    grid.sort([{"dir": 2,
    "fieldName": "ContactTitle",
    "ignoreCase": true}])
  3. Result: grouping expressions has changed

If sorting the column by clicking the sorting indicator whitin the chip in group area - DOES NOT change sorting expressions, which is expected.

  • TO DISCUSS LATER:
    Story 6: As a developer, I want to be able to disregard the sorting MODE option, by working direcdtly with the API. Example: If sorting mode is single, I still want to be able to apply multiple sorting via API.

Doing that results in multiple columns sorted, but they lack the ordering number. To discuss - Shall we display numbering order now?

@@ -582,7 +579,7 @@ export class IgxGridComponent extends IgxGridBaseDirective implements GridType,
* @hidden @internal
*/
public isDetailRecord(record) {
return record && record.detailsData !== undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember putting this check on a purpose when record was coming as undefined. I will double check if we need it.

@hanastasov hanastasov added ✅ status: verified Applies to PRs that have passed manual verification and removed 💥 status: in-test PRs currently being tested labels Apr 26, 2022
@hanastasov hanastasov self-requested a review April 26, 2022 07:53
@hanastasov hanastasov merged commit 30d984d into master Apr 27, 2022
@hanastasov hanastasov deleted the kdragieva/grid-sorting-master branch April 27, 2022 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grid: sorting ✅ status: verified Applies to PRs that have passed manual verification
Projects
None yet
5 participants