-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
detach axis rendering #224
Merged
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
It's no longer entirely accurate to call these "rendering" functions exactly, because they are not actually rendering the axis content until they're fed the detached node and also because the text content of the title will have to be factored out into a separate function.
Properly positioning the axis title requires accessing the node dimensions, so it can't be done as part of detached rendering and instead needs to be factored out into a separate function that runs at the end.
Much like axis titles, extending axis ticks across the entire chart requires access to the live DOM node, so it needs to be factored out.
With a more structured axis module in which scopes are not shared, it is now perfectly feasible to refer to the selection into which the axis is being rendered by simply calling it "selection" instead of aliasing it to a variable name which reflects the direction of the axis.
This is redundant – it selects based on a class, then adds that same class.
Group together functions that require a live DOM node in a post-render helper function.
Axis positions need to be factored out because they are used twice: first to set a transform when rendering the axis to a detached node, and again when rendering the axis title to a live node.
Tick rotation requires the size of the live DOM node in order to compute the necessary position offset, so it needs to be part of the post-render life cycle stage.
The post-render function added here also creates the proper stage at which to reimplement a greedy tick restriction algorithm. |
Merged
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.
Increase performance by rendering axes into detached nodes in memory before attaching to the live chart node.
Depending on the input specification and the exact circumstances, axis rendering can sometimes be a performance sink relative to rendering the chart data, particularly when there are a lot of ticks and not a lot of marks, and thus most of the DOM node weight is in the axes.
However, detaching requires adding a significant amount of new structure to the axis module, because some operations related to axis rendering will always fundamentally require live DOM nodes to measure. Those bits need to be retained but factored out into dedicated functions which are then run as part of a sort of "life cycle" stage at the end, once the otherwise-detached render has completed. The resulting performance improvement is frankly much more modest than I'd hoped while working this up, and this change introduces quite a lot of additional code and complexity, so it may make sense to mash it all back together at some point if this doesn't ultimately pay off. For the time being, the additional functions are certainly more organized albeit less intuitive, and will hopefully open up more paths forward in the future.