feat(datagrid): add ability to unsort datagrid colums #1617
Conversation
Thank you, 🤖 Clarity Release Bot |
|
follow up of #1612 |
valentin-mladenov
changed the title
There was a problem hiding this comment.
See my comments. Please also update the PR summary.
- It references a
clrDgEnableUnsortColumnsinput that does not exist in the current revision. - It states that isn't a breaking change, but requiring application code to opt out of unsorting is a breaking change. If we want to release this as a non-breaking change, it needs to be an opt-in feature.
| } else if (comparator) { | ||
| this._sortBy = comparator; | ||
| } else if (this.field) { | ||
| this._sortBy = new DatagridPropertyComparator(this.field); | ||
| } else { | ||
| if (comparator) { | ||
| this._sortBy = comparator; | ||
| } else { | ||
| if (this.field) { | ||
| this._sortBy = new DatagridPropertyComparator(this.field); | ||
| } else { | ||
| delete this._sortBy; | ||
| } | ||
| } | ||
| delete this._sortBy; |
There was a problem hiding this comment.
Does this just flatten the if blocks? Mixing such refactors with another significant code change can make the change harder to read and understand. Please extract this to a separate PR.
| // the Unsorted case happens when the current state is neither Asc nor Desc | ||
| case ClrDatagridSortOrder.UNSORTED: | ||
| default: | ||
| this._sort.clear(); | ||
| this._sortDirection = null; | ||
| break; |
There was a problem hiding this comment.
This case should be moved back to its original position to minimize this diff and make the change more obvious without requiring the reader to mentally compare the code.
Further, there are actually two places with the unsorted/default case listed first. I have fixed both in a separate PR: #1721.
| @@ -99,6 +99,7 @@ export class ClrDatagridColumn<T = any> | |||
| @Input('clrFilterStringPlaceholder') filterStringPlaceholder: string; | |||
| @Input('clrFilterNumberMaxPlaceholder') filterNumberMaxPlaceholder: string; | |||
| @Input('clrFilterNumberMinPlaceholder') filterNumberMinPlaceholder: string; | |||
| @Input('clrDgSortDisableUnsort') clrDgSortDisableUnsort = false; | |||
There was a problem hiding this comment.
| @Input('clrDgSortDisableUnsort') clrDgSortDisableUnsort = false; | |
| @Input('clrDgDisableUnsort') disableUnsort = false; |
The input alias is redundant (has "sort" twice) and the property name doesn't need the clrDg prefix.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Once turned on column sort can only be rotated between
ASCandDESCwhen clicking on the column, but can't be turned off.Issue Number: CDE-2439, CDE-2416
What is the new behavior?
Column sort can be turned off by clicking third time on the column.
The feature can be turned on by
clrDgEnableUnsortColumnsflag on datagrid level for entire datagrid.The feature can be turned on/off per column by
clrDgDisableUnsortflag on column level.Does this PR introduce a breaking change?
Other information