Clean up time range handling in embeddables #17718
Conversation
💚 Build Succeeded |
stacey-gammon
force-pushed
the
2018-04-clean-up-time-range-embeddables
branch
from
061128d
to
2326eff
Compare
💚 Build Succeeded |
| * @param {ContainerState} containerState | ||
| */ | ||
| render(domNode, containerState) { | ||
| this.onContainerStateChanged(containerState); | ||
| this.domNode = domNode; |
There was a problem hiding this comment.
I'm a little concerned about the this.onContainerStateChanged(containerState); in the render. I know this isn't React, but in general, I think of render as a "Make the DOM look like this" kind of function. If we are doing onContainerStateChanged here, it seems like it might be easy to get into a loop where onContainerStateChanged makes a change, which then makes a render happen, which then calls onContainerStateChanged, etc. Just a thought!
There was a problem hiding this comment.
I think your concerns are legit but this code is going to change soon with some updates @timroes is making to the loader. I think in the end it will be something like this pseudocode:
onContainerStateChanged(containerState) {
this.saveState(containerState);
this.reloadVisualization(getStateForRefresh());
}
render(dom, containerState) {
this.saveState(containerState);
this.createVisualization(domNode, getStateForFullRender());
}
Would that alleviate your concerns? It would just refactor the actual storing of state into a separate function both things could call.
stacey-gammon
force-pushed
the
2018-04-clean-up-time-range-embeddables
branch
from
2326eff
to
9a82f48
Compare
💚 Build Succeeded |
|
Any more comments or feedback @nreese or @chrisdavies ? |
|
None from me! |
|
Are you waiting for a review or did you want to add more functional tests first? |
| /** | ||
| * @param dashboard {DashboardState} | ||
| * @return {string|undefined} | ||
| * @return {string} timeRange.from |
There was a problem hiding this comment.
How come there are 2 return definitions?
stacey-gammon
force-pushed
the
2018-04-clean-up-time-range-embeddables
branch
2 times, most recently
from
2080754
to
26c68dc
Compare
💚 Build Succeeded |
- pass down container state on render rather than rely on specific function call orders.
stacey-gammon
force-pushed
the
2018-04-clean-up-time-range-embeddables
branch
from
26c68dc
to
8bc8e04
Compare
💚 Build Succeeded |
* Convert all inner time range formats to utc for consistency - pass down container state on render rather than rely on specific function call orders. * remove accidentally left in return jsdoc line
Also: pass down container state on render rather than rely on specific
function call orders.
Convert all inner time range formats to utc for consistency. don't store entire timeRange object in the redux tree.