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

Row comparator #1074

Merged
merged 5 commits into from
Oct 27, 2017
Merged

Row comparator #1074

merged 5 commits into from
Oct 27, 2017

Conversation

SPKorhonen
Copy link
Contributor

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

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

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")

  • Yes
  • [x ] No

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

Other information:

Suppresses linter warning / build error
Somewhat strange parameter order preserves backward compatibility
@@ -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) :
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@Confusedfish
Copy link
Contributor

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.

@SPKorhonen
Copy link
Contributor Author

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.

@Confusedfish
Copy link
Contributor

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 :)

Copy link
Contributor

@amcdnl amcdnl left a comment

Choose a reason for hiding this comment

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

LGTM

@amcdnl amcdnl merged commit 6492927 into swimlane:master Oct 27, 2017
@Ecksters
Copy link

Ecksters commented Nov 3, 2017

Thank you so much for this, awesome feature.

@amcdnl
Copy link
Contributor

amcdnl commented Nov 4, 2017

No problem. It’s in latest release

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