-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Row comparator #1074
Row comparator #1074
Conversation
Suppresses linter warning / build error
Somewhat strange parameter order preserves backward compatibility
src/utils/sort.ts
Outdated
@@ -83,8 +83,8 @@ export function sortRows(rows: any[], columns: any[], dirs: SortPropDir[]): any[ | |||
const propB = valueGetter(b, prop); | |||
|
|||
const comparison = cachedDir.dir !== SortDirection.desc ? | |||
cachedDir.compareFn(propA, propB) : | |||
-cachedDir.compareFn(propA, propB); | |||
cachedDir.compareFn(propA, propB, a, b, true) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment here whats going on. Also, better name for a/b would be good too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comments to the code and renamed a/b to rowA and rowB which should be less cryptic. Also I added few lines to documentation for consideration.
Thanks for this @SPKorhonen it solved my problem perfectly. I needed to place a total row into the table and didn't want to use the footer and without this it wasn't possible as the total row would jump about depending on the sorted column. |
I had a very similar use case, however I needed to bubble certain rows to the top regardless of sort. Luckily this functionality could be implemented without breaking too many things. |
Where I lost most of the time was figuring out how to build the npm package. I am not sure if there is an issue with my environment or if it is windows specific but I could not reference the package via npm link once built on an aot project. I had to resort to building in a Linux docker and npm installing the resulting .gz file. What process do you use to build and what platform are you on? Actually referencing your code took minutes :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you so much for this, awesome feature. |
No problem. It’s in latest release |
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
Column comparators get only the value of current cell. This prevents more complex comparators which would need to access to the whole row. This problem has been referred to in issue #817.
What is the new behavior?
The rows being compared are added as the 3rd and 4th parameters to the comparator function. This preserves backward compatibility as js functions silently ignore additional parameters. This PR resolves issue #817 with minimal changes.
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: