Renderer regression #496
Renderer regression #496
Conversation
.circleci/config.yml
Outdated
| @@ -27,7 +27,7 @@ jobs: | |||
| name: Install dependencies (dash) | |||
| command: | | |||
| git clone git@github.com:plotly/dash.git | |||
| git clone git@github.com:plotly/dash-renderer.git | |||
| git clone -b renderer-regression git@github.com:plotly/dash-renderer.git | |||
There was a problem hiding this comment.
To be removed once in master
| const isLoading = getLoadingState(this); | ||
|
|
||
| if (isLoading) { | ||
| if (loading_state && loading_state.is_loading) { |
There was a problem hiding this comment.
The nested loading state is already determined by the renderer. Only need to check the prop value
src/components/Loading.react.js
Outdated
| @@ -62,6 +61,8 @@ export default class Loading extends Component { | |||
| } | |||
| } | |||
|
|
|||
| Loading._dashprivate_deepstate = true; | |||
There was a problem hiding this comment.
set the deepstate flag for nested loading state
| child => child.type !== Loading && getLoadingState(child), | ||
| Array.isArray(children) ? children : [children] | ||
| ); | ||
| } |
There was a problem hiding this comment.
This is resolved by the renderer, no longer needed
| @@ -28,7 +28,7 @@ const CircleSpinner = ({ | |||
| return ( | |||
| <div style={style ? style : {}} className={spinnerClass}> | |||
| {debugTitle} | |||
| <div className="dash-sk-circle"> | |||
| <div className="dash-spinner dash-sk-circle"> | |||
There was a problem hiding this comment.
Extra class on the spinners, easier to target general spinner through css (and to automate tests)
| @@ -19,18 +19,18 @@ const GraphSpinner = ({status, fullscreen, debug, className, style}) => { | |||
| <div style={style ? style : {}} className={spinnerClass}> | |||
| <div> | |||
| {debugTitle} | |||
| <div className="dash-spinner"> | |||
| <div className="dash-spinner__bottom"> | |||
| <div className="dash-spinner dash-graph-spinner"> | |||
There was a problem hiding this comment.
dash-spinner and variants become dash-graph-spinner
| with lock: | ||
| return 'content' | ||
|
|
||
| with lock: |
There was a problem hiding this comment.
Using with lock instead of lock.acquire() / lock.release() as a test failure will throw an error and will prevent the lock from being released -- hence the test app will never terminate, blocking the tests indefinitely!
with lock:
// Code
is equivalent to
try {
lock.acquire()
// Code
} finally {
lock.release()
}
Including @plotly/dash to update previously tagged info in #486 (comment).
| @@ -216,9 +216,306 @@ def test_upload_gallery(self): | |||
|
|
|||
| self.snapshot('test_upload_gallery') | |||
|
|
|||
| def test_loading_component_initialization(self): | |||
There was a problem hiding this comment.
5 scenarios are tested here
- LC works on initial callback execution
- LC works on triggered callback
- multiple LCs have independent states
- nested LCs get pruned from parent's state
- dynamically loaded LCs work
| lock.acquire() | ||
| lock.release() | ||
| return 5 | ||
| with lock: |
There was a problem hiding this comment.
Fixing tests previously written with lock.acquire() / lock.release()
| } | ||
| </style>" | ||
| `; | ||
| exports[`Loading checks all it's children for a loading_state: Loading spinner for children 1`] = `"<div>Child 1</div><div>Child 2</div><div loading_state=\\"[object Object]\\">Child 3</div>"`; |
There was a problem hiding this comment.
new dash-spinner class changes the jest snapshots
|
|
||
| self.wait_for_element_by_css_selector( | ||
| '#horizontal-slider[data-dash-is-loading="true"]' | ||
| ) |
There was a problem hiding this comment.
Adding to the test -- we don't only check that the loading state is ok on initialization, also check that it's ok on user initiated actions
|
|
||
| self.wait_for_element_by_css_selector( | ||
| '#horizontal-range-slider[data-dash-is-loading="true"]' | ||
| ) |
There was a problem hiding this comment.
Init and user action cases, as above
There was a problem hiding this comment.
Looks great, only question is the flag name plotly/dash-renderer#140 (review)
|
Local testing: confirming that |
May require a changelog entry if this and plotly/dash-renderer#140 fixes the behavior encountered by community with dynamically loaded LCs. @valentijnnieman will want to go over that one with you.