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

fix regression + new test #63

Merged
merged 1 commit into from
Sep 10, 2018
Merged

Conversation

Marc-Andre-Rivet
Copy link
Contributor

The refactoring for fixed rows+columns introduced a regression on double click handling.
Modifying the code to do the same transform that's done on click.

Adding one test for this case.

@@ -28,4 +28,9 @@ describe('dash basic', () => {
});
});
});

it('can get cell with double click', () => {
DashTable.getCell(3, 3).within(() => cy.get('div').dblclick());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why I can click on the cell itself but need to dblclick on the child both handlers are defined on the cell itself... otherwise this does not work.

@@ -62,6 +62,8 @@
{'id': 12, 'name': 'Timely response?'},
{'id': 13, 'name': 'Consumer disputed?'}
],
n_fixed_columns=2,
n_fixed_rows=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.

Changing the test case to have fixed rows/columns as this is necessary for the bug to activate.

Copy link
Contributor

@wbrgss wbrgss left a comment

Choose a reason for hiding this comment

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

The refactoring for fixed rows+columns introduced a regression on double click handling.
Modifying the code to do the same transform that's done on click.

Is this what is meant to happen? Because with this PR single click behaviour sets a cell to focused (i.e. highlighted), and a double click sets the cell to active (i.e. editable). I'm happy with this behaviour; it's what I suggested in #62. But this comment makes it a bit unclear, unless I'm misinterpreting the meaning of "transform".

@cldougl
Copy link
Member

cldougl commented Sep 10, 2018

afaik

single click behaviour sets a cell to focused (i.e. highlighted), and a double click sets the cell to active (i.e. editable).

is what we want to aim for

@Marc-Andre-Rivet
Copy link
Contributor Author

afaik it's also what we want, this code's impact/logic was never touched.
By transform I just mean applying the offset so we hit the correct cell.

@Marc-Andre-Rivet Marc-Andre-Rivet merged commit 692d313 into master Sep 10, 2018
@Marc-Andre-Rivet Marc-Andre-Rivet deleted the 3.0-double-click-regression branch September 12, 2018 11:31
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