Ensure that components are still in the DOM when they are loading. #740

shammamah-zz · 2020-01-22T19:22:16Z

Closes #674.

Also these are all the same thing:
Closes #780
Closes #756
Closes #684

About

Since the children property of the dcc.Loading component is not rendered when loading_state.is_loading is 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 the children but with visibility: 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 the children.

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_loading changes. 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

from urllib.request import urlopen
import json
import pandas as pd
import plotly.graph_objects as go
import dash
import dash_core_components as dcc
import dash_html_components as html
from dash.dependencies import Input, Output, State
from dash.exceptions import PreventUpdate
from time import sleep


with urlopen('https://raw.githubusercontent.com/plotly/datasets/master/geojson-counties-fips.json') as response:
    counties = json.load(response)


df = pd.read_csv("https://raw.githubusercontent.com/plotly/datasets/master/fips-unemp-16.csv",
                 dtype={"fips": str})

app = dash.Dash(__name__)
server = app.server

mapbox_token = "pk.eyJ1IjoicGxvdGx5bWFwYm94IiwiYSI6ImNqdnBvNDMyaTAxYzkzeW5ubWdpZ2VjbmMifQ.TXcBE-xg9BFdV2ocecc_7g"

fig = go.Figure(go.Choroplethmapbox(geojson=counties, locations=df.fips, z=df.unemp,
                                    colorscale="Viridis", zmin=0, zmax=12,
                                    marker_opacity=0.5, marker_line_width=0))
fig.update_layout(mapbox_style="carto-positron",
                  mapbox_accesstoken=mapbox_token,
                  mapbox_zoom=3, mapbox_center={"lat": 37.0902, "lon": -95.7129})
fig.update_layout(margin={"r": 0, "t": 0, "l": 0, "b": 0}, uirevision=True)

app.layout = html.Div(
    children=[
        html.Button(id='btn', children='update', n_clicks=0),
        dcc.Loading(dcc.Graph(id='mapbox-loading', figure=fig), type='default'),
        html.Div(style={'backgroundColor': 'pink'}, children='hello')]
)


@app.callback(
    Output('mapbox-loading', 'figure'),
    [Input('btn', 'n_clicks')]
)
def update(click):
    if click is None or click == 0:
        raise PreventUpdate
    sleep(2)
    fig.update_layout(mapbox_style="light")
    return fig


if __name__ == '__main__':
    app.run_server(debug=True, port=8053)

Marc-Andre-Rivet

Missing snapshot baseline and changelog.

I expect a new test that ensures the state wasn't lost before / after the component was in loading state.

Marc-Andre-Rivet · 2020-03-02T14:35:31Z

tests/unit/Loading.test.js

+            .find('.dash-spinner')
+            .parent()
+            .html()
+    ).toMatchSnapshot('Loading with is_loading=true');


I don't see the corresponding snapshot baseline in this PR

This snapshot is already here:

dash-core-components/tests/unit/__snapshots__/Loading.test.js.snap

Line 224 in 803b67b

exports[`Loading renders: Loading with is_loading=true 1`] = `

The baseline is the same, but the test itself had to change because I moved things "down" in the DOM.

shammamah-zz · 2020-03-05T17:28:17Z

Running into some strange test failures.

This test

dash-core-components/tests/integration/graph/test_graph_basics.py

Line 36 in b35a560

def test_grbs002_wrapped_graph_has_no_infinite_loop(dash_dcc, is_eager):

fails for both values of is_eager on the CI machine. Upon running the tests locally, they both pass.

The versions of Python, all of the Python packages, and chromedriver are the same on my computer and the CI machine.

Removing this line

dash-core-components/tests/integration/graph/test_graph_basics.py

Line 88 in b35a560

sleep(1)

allows for is_eager=True to pass on the CI machine.

Relevant CI run: https://circleci.com/gh/plotly/dash-core-components/17161#tests/containers/1

alexcjohnson · 2020-04-09T17:00:27Z

Some tests are currently failing because I didn't put className onto the wrapper div when loading is done, I only put it on the spinner itself, when the spinner is present. Is it useful for this class to be applied to the wrapper? At first glance I'd think this is actually unwanted, because then you can't use this class to target the spinner.

alexcjohnson · 2020-04-09T20:37:16Z

pytest.ini

@@ -0,0 +1,5 @@
+[pytest]
+testpaths = tests/


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

alexcjohnson · 2020-04-09T20:42:25Z

tests/unit/Loading.test.js

@@ -23,7 +28,7 @@ test('Loading renders without loading_state', () => {
        </Loading>
    );

-    expect(loading.html()).toEqual('<div>Loading is done!</div>');
+    expect(loading.html()).toEqual('<div>Loading is done!</div><div></div>');


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?

alexcjohnson · 2020-04-09T20:43:09Z

tests/integration/store/test_component_props.py

    dash_dcc.start_server(store_app)

    assert dash_dcc.wait_for_contains_text("#output", store_app.uuid)

    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)


As a bare assert this gave spurious errors.

alexcjohnson · 2020-04-09T20:44:59Z

tests/integration/loading/test_loading_component.py

+
+    test_identity = (
+        "var gd_ = document.querySelector('.js-plotly-plot');"
+        "return gd_ === window.gd && gd_.__test__ === 'boo';"


gd_.__test__ === 'boo' should be redundant after we already have gd_ === window.gd to show the DOM element is unchanged, just being paranoid again...

chriddyp · 2020-04-09T20:48:00Z

src/components/Loading.react.js

-            type(this.props.children) !== 'Object' ||
-            type(this.props.children) !== 'Function'
-        ) {
-            return <div className={className}>{this.props.children}</div>;


Yeah, this part, right? Agreed, it isn't clear to me why this was there.

chriddyp · 2020-04-09T20:55:27Z

tests/integration/loading/test_loading_component.py

+
+    @app.callback(Output("div-1", "children"), [Input("root", "n_clicks")])
+    def updateDiv(children):
+        with lock:


wow very nice 💯

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

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

Marc-Andre-Rivet · 2020-04-09T20:57:08Z

tests/integration/loading/test_loading_component.py

+    with lock:
+        dash_dcc.start_server(app)
+        dash_dcc.find_element(".loading .dash-spinner")
+        dash_dcc.find_element("#graph .js-plotly-plot")


Could we also validate that the graph isn't visible while loading?

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.

If equivalent, all good :D

CHANGELOG.md

chriddyp

💃

tests/integration/loading/test_loading_component.py

Co-Authored-By: Chris Parmer <[email protected]>

…onents into 674-map-uirevision

Marc-Andre-Rivet · 2020-04-09T21:24:18Z

Seems fine in IE11, we are apparently using the part of flex that actually works

Marc-Andre-Rivet · 2020-04-09T21:27:46Z

tests/integration/loading/test_loading_component.py

        assert dash_dcc.driver.execute_script(test_identity)
+        assert get_graph_visibility() == "hidden"


Ensure that components are still in the DOM when they are loading.

8e181d7

shammamah-zz requested review from alexcjohnson and Marc-Andre-Rivet as code owners January 22, 2020 19:22

Shammamah Hossain added 3 commits January 22, 2020 14:31

Define style prop for spinners as object and not bool.

73db3ca

Fix CSS property names.

377004b

Merge branch 'dev' into 674-map-uirevision

a09a508

shammamah-zz force-pushed the 674-map-uirevision branch from 2ee39b1 to db34560 Compare January 23, 2020 19:06

Update loading component tests.

284c3a3

shammamah-zz force-pushed the 674-map-uirevision branch from db34560 to 284c3a3 Compare January 23, 2020 19:12

Shammamah Hossain and others added 3 commits February 3, 2020 11:09

Merge branch 'dev' into 674-map-uirevision

a90af15

Merge branch 'dev' into 674-map-uirevision

ceff132

Merge branch 'dev' into 674-map-uirevision

6167e15

Marc-Andre-Rivet suggested changes Mar 2, 2020

View reviewed changes

shammamah-zz and others added 3 commits March 3, 2020 11:06

Fix test name.

bfe6d15

Lock circleci python versions.

8dc4529

Merge branch 'dev' into 674-map-uirevision

1ead9bc

Shammamah Hossain and others added 8 commits March 10, 2020 12:50

Merge branch 'dev' into 674-map-uirevision

ef5d193

Update CHANGELOG.

1cb8da3

Merge branch 'dev' into 674-map-uirevision

49fc1ea

revert python version lock on CI

f985ece

pyest.ini - mainly to speed up test discovery with testpaths

01425ff

extend --pause option to dashdcc fixture

6d398a2

fix typo in test_graph_basics that somehow we didn't care about before

aa4fce7

more consistent structure for Loading component

600a09f

alexcjohnson added 4 commits April 9, 2020 14:35

robustify store test

386137e

port loading component tests to dash.testing - without fixing them

22a1975

update loading component tests for new class usage

2bfaa28

black test_loading_component

8bf9fbf

test that children retain identity inside loading components

26c6533

alexcjohnson requested a review from chriddyp April 9, 2020 20:34

alexcjohnson reviewed Apr 9, 2020

View reviewed changes

chriddyp reviewed Apr 9, 2020

View reviewed changes

Marc-Andre-Rivet reviewed Apr 9, 2020

View reviewed changes

chriddyp reviewed Apr 9, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

chriddyp approved these changes Apr 9, 2020

View reviewed changes

Marc-Andre-Rivet reviewed Apr 9, 2020

View reviewed changes

tests/integration/loading/test_loading_component.py Outdated Show resolved Hide resolved

Update CHANGELOG.md

a80f0db

Co-Authored-By: Chris Parmer <[email protected]>

Marc-Andre-Rivet approved these changes Apr 9, 2020

View reviewed changes

alexcjohnson added 2 commits April 9, 2020 17:18

include computed visibility in the loading/graph test

4fc809a

Merge branch '674-map-uirevision' of github.com:plotly/dash-core-comp…

fb15aa9

…onents into 674-map-uirevision

Marc-Andre-Rivet reviewed Apr 9, 2020

View reviewed changes

robustify store test_data_lifecycle

2b41ea0

alexcjohnson merged commit a42d7d2 into dev Apr 9, 2020

alexcjohnson deleted the 674-map-uirevision branch April 9, 2020 21:44

Marc-Andre-Rivet mentioned this pull request Apr 14, 2020

Issue 384 - Handling of None data and columns props plotly/dash-table#731

Merged

bcliang mentioned this pull request Jun 16, 2020

Sync with latest dash-core-components bcliang/dash-extendable-graph#36

Closed

wbrgss mentioned this pull request Jul 8, 2020

Graph dimension problem with dcc.Loading — regression (?) in DCC 1.9.1 #831

Closed

wbrgss mentioned this pull request Aug 6, 2020

Allow parent_className and parent_style attributes on dcc.Loading components #840

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

shammamah-zz commented Jan 22, 2020 •

edited by alexcjohnson

Loading

Marc-Andre-Rivet left a comment •

edited

Loading

Marc-Andre-Rivet Mar 2, 2020

shammamah-zz Mar 3, 2020

shammamah-zz commented Mar 5, 2020

alexcjohnson commented Apr 9, 2020

alexcjohnson Apr 9, 2020

Marc-Andre-Rivet Apr 9, 2020

alexcjohnson Apr 9, 2020

alexcjohnson Apr 9, 2020

alexcjohnson Apr 9, 2020

chriddyp Apr 9, 2020

chriddyp Apr 9, 2020

alexcjohnson Apr 9, 2020

Marc-Andre-Rivet Apr 9, 2020 •

edited

Loading

Marc-Andre-Rivet Apr 9, 2020 •

edited

Loading

alexcjohnson Apr 9, 2020

Marc-Andre-Rivet Apr 9, 2020

chriddyp left a comment

Marc-Andre-Rivet commented Apr 9, 2020 •

edited

Loading

Marc-Andre-Rivet Apr 9, 2020

		assert dash_dcc.driver.execute_script(test_identity)
		assert get_graph_visibility() == "hidden"

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

Conversation

shammamah-zz commented Jan 22, 2020 • edited by alexcjohnson Loading

About

Before

After

Sample app code

Marc-Andre-Rivet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shammamah-zz commented Mar 5, 2020

alexcjohnson commented Apr 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marc-Andre-Rivet Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Marc-Andre-Rivet Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriddyp left a comment

Choose a reason for hiding this comment

Marc-Andre-Rivet commented Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

shammamah-zz commented Jan 22, 2020 •

edited by alexcjohnson

Loading

Marc-Andre-Rivet left a comment •

edited

Loading

Marc-Andre-Rivet Apr 9, 2020 •

edited

Loading

Marc-Andre-Rivet Apr 9, 2020 •

edited

Loading

Marc-Andre-Rivet commented Apr 9, 2020 •

edited

Loading