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

IE11 compatibility #637

Merged
merged 45 commits into from
Nov 14, 2019
Merged

IE11 compatibility #637

merged 45 commits into from
Nov 14, 2019

Conversation

Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Nov 8, 2019

Sibling of plotly/dash#1006

Also fixes #641

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 00:29 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 13:53 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 14:19 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 16:41 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 16:43 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 16:49 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 17:01 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 17:13 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 17:18 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 17:20 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 17:29 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 17:43 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 17:47 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 8, 2019 17:53 Inactive
@@ -410,6 +411,7 @@ export type SanitizedProps = Omit<Omit<
Merge<PropsWithDefaults, {
fixed_columns: number;
fixed_rows: number;
loading_state: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Synthetize the loading_state object to a boolean representing whether data is being worked on. Prevents a bunch of useless re-renders.

@@ -115,7 +115,6 @@ describe('select row', () => {
expectCellSelection([1, 2, 3], [3001, 3002, 3003], [0, 1], [0, 1], [2, 1], [2, 1]);

// shrink the selection
DOM.focused.type(Key.Shift, { release: false });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cypress 3.6.x API changed in subtle ways, doing two consecutive non-release shift key presses is actually two presses, needs to be removed for the selection to behave as expected.

@@ -14,6 +14,7 @@
// ***********************************************************

// Import commands.js using ES2015 syntax:
import '@babel/polyfill';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The server build gets polyfills from the renderer, the standalone build adds it to the entrypoint. Unit tests get it from cypress, here.

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 12, 2019 23:00 Inactive
if (!currentApplyFocus) {
nextState.applyFocus = true;
}
} else if (nextProps.loading_state !== this.props.loading_state) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you're going to have to explain this one to me... why does entering or exiting loading trigger this check?

Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Nov 13, 2019

Choose a reason for hiding this comment

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

If a callback modifying data is triggered and the focus is on a table cell, the table needs to be re-rendered once the callback returns to make the cell writeable again.

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 13, 2019 13:24 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 13, 2019 13:31 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 13, 2019 13:46 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 13, 2019 18:41 Inactive
.find('.dash-input-cell-value-container > input').should('have.length', 1);
.within(() => cy.get('.dash-cell-value')
.should('have.value', 'Hello'));
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the test a bit tighter by skipping Enter / Click, gives the table less chance to render multiple times and hide issues for renders with a smaller footprint (less props changed)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not excited about the cy.wait(5000) statements that are proliferating here... I won't block on this, given all the cypress headaches already in this PR, but it would be great if at some point we could figure out a condition to wait for rather than a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverting. I ran this in 3.4.1 but in 3.6.1 I'm encountering weird timing issues.

Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Nov 13, 2019

Choose a reason for hiding this comment

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

About the cy.wait(5000) the callback waits for 5 seconds and the maximum default timeout for a selector to succeed in Cypress is 4000ms. It is a bit yucky. Maybe we could rewrite the tests with a timeout override or make the callback shorter -- as opposed to the Selenium tests, these tests don't have access to the Python callback. One definite disadvantage to running e2e tests in Cypress.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the extra tests of loading/editing. Once these tests all pass 😅 this looks ready to me! 💃

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 13, 2019 19:33 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 13, 2019 19:34 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 13, 2019 20:37 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-637 November 14, 2019 15:00 Inactive
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.

Table loading-state behaves incorrectly
3 participants