Ensure that components are still in the DOM when they are loading.#740
Ensure that components are still in the DOM when they are loading.#740alexcjohnson merged 28 commits intodevfrom
Conversation
2ee39b1 to
db34560
Compare
db34560 to
284c3a3
Compare
| .find('.dash-spinner') | ||
| .parent() | ||
| .html() | ||
| ).toMatchSnapshot('Loading with is_loading=true'); |
There was a problem hiding this comment.
I don't see the corresponding snapshot baseline in this PR
There was a problem hiding this comment.
This snapshot is already here:
The baseline is the same, but the test itself had to change because I moved things "down" in the DOM.
|
Running into some strange test failures. This test fails for both values ofis_eager on the CI machine. Upon running the tests locally, they both pass.
The versions of Python, all of the Python packages, and Removing this line allows foris_eager=True to pass on the CI machine.
Relevant CI run: https://circleci.com/gh/plotly/dash-core-components/17161#tests/containers/1 |
|
Some tests are currently failing because I didn't put |
| @@ -0,0 +1,5 @@ | |||
| [pytest] | |||
| testpaths = tests/ | |||
There was a problem hiding this comment.
having this line cuts test discovery time locally from ~10 seconds to <1sec - particularly nice when calling pytest -k ...
I didn't pay attention to the other lines, just copied from dash
| ); | ||
|
|
||
| expect(loading.html()).toEqual('<div>Loading is done!</div>'); | ||
| expect(loading.html()).toEqual('<div>Loading is done!</div><div></div>'); |
There was a problem hiding this comment.
The spinner now leaves its wrapper div around even after it disappears. I did it this way to ensure React had no chance of being confused about the identity of the real children components - loading or not doesn't change the immediate children of the outer containing div at all. Maybe overly paranoid, but didn't seem like it would hurt, right?
|
|
||
| dash_dcc.multiple_click("#btn", 3) | ||
| assert dash_dcc.get_local_storage() == {"n_clicks": 3} | ||
| wait.until(lambda: dash_dcc.get_local_storage() == {"n_clicks": 3}, timeout=1) |
There was a problem hiding this comment.
As a bare assert this gave spurious errors.
|
|
||
| test_identity = ( | ||
| "var gd_ = document.querySelector('.js-plotly-plot');" | ||
| "return gd_ === window.gd && gd_.__test__ === 'boo';" |
There was a problem hiding this comment.
gd_.__test__ === 'boo' should be redundant after we already have gd_ === window.gd to show the DOM element is unchanged, just being paranoid again...
| type(this.props.children) !== 'Object' || | ||
| type(this.props.children) !== 'Function' | ||
| ) { | ||
| return <div className={className}>{this.props.children}</div>; |
There was a problem hiding this comment.
Yeah, this part, right? Agreed, it isn't clear to me why this was there.
|
|
||
| @app.callback(Output("div-1", "children"), [Input("root", "n_clicks")]) | ||
| def updateDiv(children): | ||
| with lock: |
There was a problem hiding this comment.
Only the last test here (006) is new, the others I just pulled out of test_integration_1.py into the dash.testing format before fixing them for the className update
There was a problem hiding this comment.
@HammadTheOne, @harryturr When writing tests involving callbacks, using a Lock like ☝️ is not always necessary but is the best practice. Gives you full control of when the callback and your test conditions will be run in respect to one another.
| with lock: | ||
| dash_dcc.start_server(app) | ||
| dash_dcc.find_element(".loading .dash-spinner") | ||
| dash_dcc.find_element("#graph .js-plotly-plot") |
There was a problem hiding this comment.
Could we also validate that the graph isn't visible while loading?
There was a problem hiding this comment.
I did that with some of the simpler items eg:
dash_dcc.wait_for_text_to_equal("#btn-3", "")
it looks like it's empty even though it actually still has text in it. Not sure exactly how to do that for a graph though - I suppose I could look directly at the visibility style on the element that sets it to hidden.
There was a problem hiding this comment.
If equivalent, all good :D
Co-Authored-By: Chris Parmer <chris@plot.ly>
…onents into 674-map-uirevision
| assert len(dash_dcc.find_elements('.js-plotly-plot .bars path')) == 3 | ||
| assert len(dash_dcc.find_elements(".js-plotly-plot .bars path")) == 3 | ||
| assert dash_dcc.driver.execute_script(test_identity) | ||
| assert get_graph_visibility() == "hidden" |

Closes #674.
Also these are all the same thing:
Closes #780
Closes #756
Closes #684
About
Since the
childrenproperty of thedcc.Loadingcomponent is not rendered whenloading_state.is_loadingis true, this leads to some information about user interactions being lost when a graph (specifically, a map) is wrapped in the loading component. In this PR, we still render thechildrenbut withvisibility: hidden, and display the spinner over top of it. The component is not visible, but it still takes up the same amount of space when it is loading, which avoids issues that have to do with divs shifting up or down during loading if the spinner is smaller or larger than thechildren.We might want to instead always return this "wrapper" div that contains both the spinner and the component, and only change the visibility when
loading_state.is_loadingchanges. One issue that stems from this is that the component itself would be moved "down" a level in the DOM, which might affect the CSS/layout of different apps.Before
After
Sample app code