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
Merged

Sorting sorts ascending first and displays correct icon #164

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5be5338
Refactor isFrontEnd and isBackEnd to class methods
valentijnnieman Oct 23, 2018
d03c69d
Flip sorting icons and ascend first
valentijnnieman Oct 23, 2018
1a0ab20
Remove debugging statements
valentijnnieman Oct 23, 2018
3215fea
Bump version to rc7
valentijnnieman Oct 23, 2018
aa1d8a7
Fix sorting and delete_row tests
valentijnnieman Oct 23, 2018
474f965
Fix copy_paste and delete_row tests
valentijnnieman Oct 23, 2018
f4c16c5
Fix select_row_tests
valentijnnieman Oct 23, 2018
96a6979
Refactor standalone/delete_row_test to sort twice
valentijnnieman Oct 23, 2018
ba9ef76
Rebuild bundle
valentijnnieman Oct 23, 2018
756652d
Refactor isFrontEnd and isBackEnd functions outside of class
valentijnnieman Oct 24, 2018
d0acd81
Merged with master & fixed merge conflicts, bumped version & changelog
valentijnnieman Oct 24, 2018
8025e27
Fixed dash_table/package.json version
valentijnnieman Oct 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,4 +390,8 @@ Derived properties allow the component to expose complex state that can be usefu
dataframe_previous -> data_previous
dataframe_timestamp -> data_timestamp
derived_virtual_dataframe -> derived_virtual_data
derived_viewport_datafram -> derived_viewport_data
derived_viewport_datafram -> derived_viewport_data

## 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.

4,928 changes: 4,910 additions & 18 deletions dash_table/bundle.js
View file
Open in desktop

Large diffs are not rendered by default.

4,986 changes: 4,968 additions & 18 deletions dash_table/demo.js
View file
Open in desktop

Large diffs are not rendered by default.

27 changes: 26 additions & 1 deletion dash_table/metadata.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,32 @@
"src/dash-table/Table.js": {
"description": "",
"displayName": "Table",
"methods": [],
"methods": [
{
"name": "isFrontEnd",
"docblock": null,
"modifiers": [],
"params": [
{
"name": "value",
"type": null
}
],
"returns": null
},
{
"name": "isBackEnd",
"docblock": null,
"modifiers": [],
"params": [
{
"name": "value",
"type": null
}
],
"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.

"props": {
"active_cell": {
"type": {
Expand Down
2 changes: 1 addition & 1 deletion dash_table/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "dash-table",
"version": "3.1.0rc6",
"version": "3.1.0rc7",
"description": "Dash table",
"main": "build/index.js",
"scripts": {
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "dash-table",
"version": "3.1.0rc6",
"version": "3.1.0rc7",
"description": "Dash table",
"main": "build/index.js",
"scripts": {
Expand Down
26 changes: 16 additions & 10 deletions src/dash-table/Table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,29 @@ import 'dash-table/style/component.less';
import Logger from 'core/Logger';

export default class Table extends Component {
constructor(props) {
super(props);

this.isFrontEnd = this.isFrontEnd.bind(this);
this.isBackEnd = this.isBackEnd.bind(this);
}
isFrontEnd(value) {
return ['fe', true, false].indexOf(value) !== -1;
}

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 •
edited
Loading

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 👍


render() {
const {
filtering,
sorting,
pagination_mode
} = this.props;

function isFrontEnd(value: any) {
return ['fe', true, false].indexOf(value) !== -1;
}

function isBackEnd(value: any) {
return ['be', false].indexOf(value) !== -1;
}

const isValid = isFrontEnd(pagination_mode) ||
(isBackEnd(filtering) && isBackEnd(sorting));
const isValid = this.isFrontEnd(pagination_mode) ||
(this.isBackEnd(filtering) && this.isBackEnd(sorting));

if (!isValid) {
Logger.error(`Invalid combination of filtering / sorting / pagination`, filtering, sorting, pagination_mode);
Expand Down
12 changes: 7 additions & 5 deletions src/dash-table/derived/header/content.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,16 @@ function doSort(columnId: ColumnId, sortSettings: SortSettings, sortType: Sortin
let direction: SortDirection;
switch (getSorting(columnId, sortSettings)) {
case SortDirection.Descending:
direction = SortDirection.Ascending;
direction = SortDirection.None;
break;
case SortDirection.Ascending:
direction = SortDirection.None;
direction = SortDirection.Descending;
break;
case SortDirection.None:
direction = SortDirection.Ascending;
break;
default:
direction = SortDirection.Descending;
direction = SortDirection.Ascending;
break;
}

Expand Down Expand Up @@ -68,9 +70,9 @@ function getSorting(columnId: ColumnId, settings: SortSettings): SortDirection {
function getSortingIcon(columnId: ColumnId, sortSettings: SortSettings) {
switch (getSorting(columnId, sortSettings)) {
case SortDirection.Descending:
return '↑';
case SortDirection.Ascending:
return '↓';
case SortDirection.Ascending:
return '↑';
case SortDirection.None:
default:
return '↕';
Expand Down
6 changes: 3 additions & 3 deletions tests/cypress/tests/server/copy_paste_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ describe('copy paste', () => {
cy.get('tr th.column-2 .sort').last().click();

DashTable.getCell(0, 0).click();
DashTable.getCell(0, 0).within(() => cy.get('.dash-cell-value').should('have.value', '103'));
DashTable.getCell(0, 0).within(() => cy.get('.dash-cell-value').should('have.value', '11'));

DOM.focused.type(`${Key.Meta}c`);

Expand All @@ -113,7 +113,7 @@ describe('copy paste', () => {
.within(() => cy.get('.dash-cell-value').should('have.html', 'MODIFIED'));
DashTable
.getCell(1, 0)
.within(() => cy.get('.dash-cell-value').should('have.value', '103'));
.within(() => cy.get('.dash-cell-value').should('have.value', '11'));

DashTable.getCell(1, 1).click();
DOM.focused.type(`${Key.Meta}c`);
Expand All @@ -126,7 +126,7 @@ describe('copy paste', () => {
.within(() => cy.get('.dash-cell-value').should('have.value', 'MODIFIED'));
DashTable
.getCell(1, 0)
.within(() => cy.get('.dash-cell-value').should('have.html', '103'));
.within(() => cy.get('.dash-cell-value').should('have.html', '11'));
});
});
});
2 changes: 1 addition & 1 deletion tests/cypress/tests/server/delete_row_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ describe('delete', () => {
});

it('can delete row when sorted', () => {
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 });
DashTable.getCell(0, 0).within(() => cy.get('.dash-cell-value').should('have.html', '28155'));
DashTable.getDelete(0).click();
DashTable.getCell(0, 0).within(() => cy.get('.dash-cell-value').should('have.html', '28154'));
Expand Down
2 changes: 1 addition & 1 deletion tests/cypress/tests/standalone/delete_row_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ describe('delete', () => {
});

it('can delete row when sorted', () => {
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 });
DashTable.getCell(0, 0).within(() => cy.get('.dash-cell-value').should('have.html', '4999'));
DashTable.getDelete(0).click();
DashTable.getCell(0, 0).within(() => cy.get('.dash-cell-value').should('have.html', '4998'));
Expand Down
2 changes: 1 addition & 1 deletion tests/cypress/tests/standalone/select_row_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

DashTable.getSelect(0).within(() => cy.get('input').should('not.be.checked'));
});
});
Expand Down
8 changes: 4 additions & 4 deletions tests/cypress/tests/standalone/sorting_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ describe('sort', () => {
DashTable.getCell(2, 6).within(() => cy.get('.Select-value-label').should('have.html', 'Tropical Beaches'));
DashTable.getCell(3, 6).within(() => cy.get('.Select-value-label').should('have.html', 'Humid'));
cy.get('tr th.column-6 .sort').last().click();
DashTable.getCell(0, 6).within(() => cy.get('.Select-value-label').should('have.html', 'Wet'));
DashTable.getCell(1, 6).within(() => cy.get('.Select-value-label').should('have.html', 'Wet'));
DashTable.getCell(2, 6).within(() => cy.get('.Select-value-label').should('have.html', 'Wet'));
DashTable.getCell(3, 6).within(() => cy.get('.Select-value-label').should('have.html', 'Wet'));
DashTable.getCell(0, 6).within(() => cy.get('.Select-value-label').should('have.html', 'Humid'));
DashTable.getCell(1, 6).within(() => cy.get('.Select-value-label').should('have.html', 'Humid'));
DashTable.getCell(2, 6).within(() => cy.get('.Select-value-label').should('have.html', 'Humid'));
DashTable.getCell(3, 6).within(() => cy.get('.Select-value-label').should('have.html', 'Humid'));
});
});