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

dcc.Graph: Improve responsiveness, expose resize status, allow wrapper extension #706

Merged
merged 53 commits into from
Dec 16, 2019

Conversation

Marc-Andre-Rivet
Copy link
Contributor

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

Fixes #605

Context
(1) Graph resize
Plotly.js provides graph resize on window.resize event but does not provide graph resize on parent element resize. This is consistent with other similar libraries.

Graph resize is affected by many props on the graph figure and config, namely:

  • figure.layout.autosize, figure.layout.height, figure.layout.width
  • config.responsive

The overall behavior is complex and can be summarized by these checks:
https://github.com/plotly/dash-core-components/blob/605-graph-resize/tests/integration/graph/test_graph_responsive.py#L101
https://github.com/plotly/dash-core-components/blob/605-graph-resize/tests/integration/graph/test_graph_responsive.py#L107

(2) Extendable figure/config processing in wrapped component
In certain use cases, the dcc.Graph is wrapped inside another component and provides a service rather than being the desired component itself. In these situations the wrapper component may want to override the graph's figure/config processing with logic of its own.

(3) Graph resizing / loading / asyncing... not ready in general
In certain use cases, the application requires the dcc.Graph to be fully loaded, renderered, and resized before some other operation is made (e.g. QA / testing).


Proposed Changes

  1. Extend the resize behavior of the Plotly.js graph in Dash so as to apply it on parent resize that is not triggered by a window.resize event
  2. Allow users of Dash to override the behavior of the graph by passing a new prop responsive that will override the graph responsiveness. responsive allows 3 values: True, False, 'auto' (default) that will (True) force the graph to be responsive, (False) force the graph to be non-responsive, ('auto') use the graph's default behavior and make the graph responsive on parent resize if applicable
  3. Add two function props: _dashprivate_transformFigure and _dashprivate_transformConfig that will override default processing of figure and config (https://github.com/plotly/dash-core-components/blob/605-graph-resize/src/fragments/Graph.react.js#L142)
  4. The graph will re-evaluate the figure/config when _dashprivate_transformFigure or _dashprivate_transformConfig is updated (e.g. generate a new function when the params change)
  5. The component is responsible for making sure that the functions at 3 and 4 respect the immutability of figure and config. These values should be cloned and the modified clone returned by the function.
  6. Expose the graph's state by adding/removing a dash-graph--pending class to the wrapper div of the component

** Changed unrelated to responsiveness **

  1. While updating the Plotly.js version, used the opportunity to clean up the build process in regardsto the plotly.min.js dependency

Side-effects

  1. The addition of the resize observer div requires the graph div to be wrapped up with the resize observer div inside of another div for styling reasons. To preserve backward compatibility with previous selectors, the wrapper div will in addition of having the class dash-graph, have js-plotly-plot, the class added by Plotly.js on graph divs.

Dependencies and other considerations

  1. Requires Plots.resize: promises should always be resolved plotly.js#4392 to be released
  2. It was discussed and decided outside the scope of this PR discussion that graph resizing on parent element resize should not be handled by Plotly.js -- related historical items Plotly Not Responsive When Parent Size Changes plotly.js#3984, add support for responsive charts plotly.js#2974
  3. Testing for this feature is done by testing all combinations of (responsive, autoresize, height/width, dash responsive override) and making sure that the initial and resized graph have the expected size
  4. The solution must work in IE11

Todo

  • Unify graph state for (1) async, (2) 1st render, (3) resize -- for example dash-graph-pending flag / class -- need to validate the class gets added prior to the resize starting, there might be a need for deferring resize inside a setTimeout(..., 0)
  • IE11 support? Seems to be 👌, changing flex into hard-coded heights and percentages, the graphs are responsive and all.
  • docs (https://github.com/plotly/dash-docs/issues/720) -- tagged for v1.8 also

Use variation of https://github.com/plotly/dash-core-components/blob/605-graph-resize/tests/integration/graph/test_graph_responsive.py#L23 to manually test scenarios.

* responsive (True) or not (False) based on the values in `config.responsive`,
* `figure.layout.autosize`, `figure.layout.height`, `figure.layout.width`.
*/
responsive: PropTypes.oneOf([true, false, 'auto']),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allow users to define the behavior they want for their graph, allowing them to override the normal behavior for their config/figure values.

@@ -491,6 +505,7 @@ PlotlyGraph.defaultProps = {
layout: {},
frames: [],
},
responsive: 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.

Defaults to responsive False which is the current behavior.

const DEFAULT_CONFIG = {
responsive: 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.

When responsive is true, whether through true or auto, will apply these values to config/figure to get the desired behavior.


if (type(responsive) === 'Boolean') {
return responsive;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If responsive is a boolean, use as-is. Otherwise, determined auto responsiveness by looking at config/figure for the graph.

refreshOptions={{trailing: true}}
refreshRate={50}
onResize={this.graphResize}
/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

element resize observer - either uses ResizeObserver in newer browsers, or polyfills with a DOM/css method if it doesn't. Debounce / trail / wait to prevent making useless calls.

data-dash-is-loading={
(loading_state && loading_state.is_loading) || undefined
}
style={style}
className={className}
/>
>
Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

The graph is now wrapped in a div that applies the styling -- the graph div itself has consistent styling. This appears to help responsiveness to work correctly in flex flows.

  • Need to make sure this is truly useful, not development side-effects.

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 still concerned about adding js-plotly-plot to the outer div - seems like having two nested divs with this class could lead to strange behavior, if it's actually getting used for something in CSS. Can you explain the reasoning here?

Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Dec 11, 2019

Choose a reason for hiding this comment

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

Understandable. My goal here is for old versions of DDK to continue having the same behavior. Given the way DDK styling works and the stated goal of minimizing custom CSS, this should be fine. To break this behavior would require users to assign display: flex; to js-plotly-plot. The next DDK release will be using .dash-graph.

For the DCC world, this is probably a non-issue, the general idea is that we support classes & attributes with the dash- prefix, everything else is implementation details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't add js-plotly-plot, dash>=1.8.0 will be a breaking version.
If we do add it, we can always create an issue to remove it and tag that issue as Dash v2.x.

Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Dec 16, 2019

Choose a reason for hiding this comment

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

As pointed out by @alexcjohnson, in the end the legacy
js-plotly-plot is not necessary as old ddk uses the forked version of the graph anyway.

Discussing this with @wbrgss, it seems like the only scenario left is a new version of DDK pointing to an old version of Dash, but that can be handled through CSS with *:not(.dash-graph) > .js-plotly-js in DDK itself.

@Marc-Andre-Rivet Marc-Andre-Rivet changed the title [WIP] Improve responsiveness of dcc.Graph Nov 21, 2019
@Marc-Andre-Rivet Marc-Andre-Rivet changed the title Improve responsiveness of dcc.Graph [WIP] Improve responsiveness of dcc.Graph Nov 21, 2019
@nicolaskruchten
Copy link
Contributor

I would recommend adding px.defaults.height = None to the testing sandbox to see if it makes certain things unnecessary. I'm open to the idea that PX is too opinionated about height if it's causing issues :) It certainly seems to fix the flash-of-too-tall-chart and the charts-are-too-tall-on-callback things.

@Marc-Andre-Rivet
Copy link
Contributor Author

Apart from waiting for the release from Plotly.js, the last remaining contention point might be the use of js-plotly-plot on the wrapper class for legacy purposes. Created https://github.com/plotly/dash-docs/issues/728 to update the documentation with information on the basic assumptions used when coding against classes in Dash.

- rework plotly.min.js inclusion and update logic
.gitignore Outdated
@@ -17,6 +17,7 @@ venv/

/build
/dash_core_components
/dash_core_components_base/plotly.min.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned up generation a bit to copy file directly from node_modules instead of keeping a to-be-synced copy in the py generation base folder

### Updated
- [#706](https://github.com/plotly/dash-core-components/pull/706)
- Upgraded plotly.js to [1.51.3](https://github.com/plotly/plotly.js/releases/tag/v1.51.3)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plotly.js 1.51.3 with the promise fix 🎉

2. Run `npm install` followed by `npm run build`, this will ensure the latest version of Plotly.js is in `node_modules` and copy
that version over with the other build artifacts
3. Update `dash_core_components_base/__init__.py` plotly.js `relative_package_path` and `external_url`
2. Run `npm install` followed by `npm run build`, the Plotly.js artifact will be copied and bundled over as required
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plotly.js artifact is now always plotly.min.js, no need for weird updates

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 clean clean clean 😉
Nice build simplifications, thanks!
💃

);
});

ControlledPlotlyGraph.propTypes = PropTypes.any;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Marc-Andre-Rivet I'm seeing a funny warning message in debug mode:

The prop `isRequired` is marked as required in `<<anonymous>>`,
but its value is `undefined`

That I think is coming from here... Should this just be a copy of PlotlyGraph.propTypes perhaps?

@bcliang
Copy link
Contributor

bcliang commented Jun 28, 2020

@Marc-Andre-Rivet -- Apologies for the thread dig.

Re: the side-effect of this PR... it also breaks the ability for end users to modify the contained plotly figure using clientside callbacks. Why? Client-side "UI" changes are much faster and provides a better user experience for things like changing axis on ranges, updating titles, etc.

Specifically, I've been using a this approach to trigger relayout via Plotly.relayout('graph-id', {..'layout-property':'value'; ..}.

This problem is resolved by shifting the component's id and key assignment into the nested div (i.e. the direct parent of <div class="plot-container plotly">). I created a simple integration test to check clientside callbacks in my fork (https://github.com/bcliang/dash-core-components/tree/fix-plotly-clientside-callbacks) which seems to work as expected.

However, running the full test suite, I get some flaky results (only passes on first run) due to parameterized eager_loading. Not sure if you have any thoughts.

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.

[BUG] Graph(style='height=100%') fails when reducing window size on Firefox
4 participants