Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

Sorting sorts ascending first and displays correct icon #164

Merged
merged 12 commits into from
Oct 24, 2018

Conversation

valentijnnieman
Copy link
Contributor

Closes #118 - this flips the icons so that they display down-arrow on descending, and up arrow on ascending, and selects ascending sort at first click.

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-164 October 23, 2018 17:30 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-164 October 23, 2018 17:34 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-164 October 23, 2018 17:38 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-164 October 23, 2018 17:52 Inactive

isBackEnd(value) {
return ['be', false].indexOf(value) !== -1;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't really part of the issue this PR fixes, but it's a nice fix anyway. Note that the way we have our webpack and babel configs set up now, using arrow functions as class methods throws an error upon building, hence the .bind(this) syntax, which we should get rid of at some point.

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet Oct 24, 2018

Choose a reason for hiding this comment

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

See comment above in metadata.json, otherwise 👍

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-164 October 23, 2018 18:35 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-164 October 23, 2018 18:44 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-164 October 23, 2018 18:57 Inactive
@@ -18,7 +18,7 @@ describe('select row', () => {
it('select, sort, new row is not selected', () => {
DashTable.getSelect(0).within(() => cy.get('input').click());
DashTable.getSelect(0).within(() => cy.get('input').should('be.checked'));
cy.get('tr th.column-0 .sort').last().click({ force: true });
cy.get('tr th.column-0 .sort').last().click({ force: true }).click({ force: true });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double-click to sort twice so the values actually change

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, this is the only case where it matters throughout the existing tests as here the derived_viewport_data was still in the same order as the original data.

Copy link
Member

@chriddyp chriddyp left a comment

Choose a reason for hiding this comment

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

image

It seems pretty crazy that this small number of changes would've caused such a big diff in the bundle. Are we missing anything to have reproducable builds, like:

  • A package-lock ?
  • Stricter node/npm versions?

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-164 October 23, 2018 19:52 Inactive
@valentijnnieman
Copy link
Contributor Author

@chriddyp I included the test bundle, which I didn't know was different, my mistake. Rebuild it, should be good now!

## RC7 - Sort ascending on first click
- Sorts ascending when first clicked
- Flips icons displayed so that they are pointing up on ascending and down on descending.
Copy link
Contributor

Choose a reason for hiding this comment

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

Include a link to the issue here since we have it.

],
"returns": null
}
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! I think that's exactly why I defined this methods in the render function originally!
Since they do not require 'this' they could be moved outside the class entirely, solving this issue as well.

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

  • merge with master
  • rework isFrontEnd / isBackEnd change so as to keep metadata.json file clean

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-164 October 24, 2018 16:41 Inactive
@valentijnnieman valentijnnieman merged commit 78c326a into master Oct 24, 2018
@Marc-Andre-Rivet Marc-Andre-Rivet deleted the 118-ascending-sort branch July 18, 2019 12:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants