[READY] Update SuperChart onRenderXXX listeners #6376
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #6376 +/- ##
=======================================
Coverage 73.37% 73.37%
=======================================
Files 67 67
Lines 9585 9585
=======================================
Hits 7033 7033
Misses 2552 2552Continue to review full report at Codecov.
|
kristw
changed the title
williaster
reviewed
Nov 15, 2018
| // Extends the behavior of LoadableComponent | ||
| // generated by react-loadable | ||
| // to provide post-render listeners | ||
| class CustomLoadableRenderer extends LoadableRenderer { |
There was a problem hiding this comment.
this seems a little heavy to put inside the component, inside a selector. Could we instead create a factory as an external file and pass it config? it's a bummer they don't have a post-load hook.
There was a problem hiding this comment.
Yeah. Wish they have a post-load hook. This is a bit fragile if they change how it is written.
Anyway, good call on separating into another factory file. Will do that.
| if (skipRendering || !chartProps) { | ||
| // Do not render if chartProps is not available. | ||
| // but the pre-loading has been started in this.createLoadableRenderer | ||
| // to prepare for rendering once chartProps becomes available. |
kristw
force-pushed
the
kristw-super-chart
branch
2 times, most recently
from
dfdc861 to
6579655
Compare
|
Updated and ready for review |
williaster
approved these changes
Nov 20, 2018
superset/assets/spec/javascripts/visualizations/core/createLoadableRenderer_spec.jsx
Outdated
Show resolved
Hide resolved
superset/assets/src/visualizations/core/components/createLoadableRenderer.js
Show resolved
Hide resolved
Squashed commits: [f1614c7c] extract createLoadableRenderer into a separate function [8689bc94] extend LoadableRenderer [3d04ff2b] remove skipRendering
kristw
force-pushed
the
kristw-super-chart
branch
from
6579655 to
36faec0
Compare
bipinsoniguavus
pushed a commit
to ThalesGroup/incubator-superset
that referenced
this pull request
Dec 26, 2018
* Revise LoadableRenderer (+3 squashed commits) Squashed commits: [f1614c7c] extract createLoadableRenderer into a separate function [8689bc94] extend LoadableRenderer [3d04ff2b] remove skipRendering * add number of times function was called to the test
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will improve the accuracy of rendering time logging and also resolve the issue due to out-of-react-cycle execution.
skipRenderingflag. Check for existence ofchartPropsinstead.onRenderXXXfrom the correct place. Before, I was dispatching this fromSuperChartwhich does not have full transparency to when the rendering actually happens because the rendering happens in a nested component created byreact-loadable. This PR extends the nested component generated byreact-loadableto provide listeners after rendering and use that to triggeronRenderSuccessandonRenderFailuremore accurately.preloadingof lazy-loaded vis code right afterSuperChartknows thechartType. The previous code won't load untilchartPropsis available@mistercrunch @williaster @graceguo-supercat @michellethomas @xtinec @conglei