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

Add class to `colorbar`; extract out class names #2139

Merged
merged 2 commits into from Nov 2, 2017

Conversation

Projects
None yet
2 participants
@monfera
Member

monfera commented Nov 2, 2017

See the separate commits.

@etpinard

This comment has been minimized.

Show comment
Hide comment
@etpinard

etpinard Nov 2, 2017

Member

Can I ask why? Looks good though.

Member

etpinard commented Nov 2, 2017

Can I ask why? Looks good though.

@monfera

This comment has been minimized.

Show comment
Hide comment
@monfera

monfera Nov 2, 2017

Member

@etpinard crossfilter mostly operates via the regular API, eg. Plotly.addTracesfor adding a salient trace that shows the selected points (will likely switch to the persistent selection for this), but not always. For example, adding a salient trace will duplicate the colorbar, but the original one shouldn't be shown. It is possible to Plotly.restyle the original trace to make the colorbar go away, and another Plotly.restyle to make it come back. These calls are expensive and the dashboard is quite slow as it is. To meet functional needs at decent speed, there are some shortcuts, mostly, setting the opacity of something to zero and back. These calls are infinitely fast, just one selection.style.

While this relies on implementation detail - the scenegraph - , it's unexposed and internal to the crossfilter code ie. we can trivially remove these once there is a fast, single-call API (such as Plotly.react as you suggest) that can take care of all changes with no duplicated overhead.

In the process, I saw a missing classname and thought it's good practice to add a class name anyway (and extract out the class names). As a matter of convention I automatically add class names to scenegraph elements because not doing so might introduce shallow, avoidable gotchas during dev, and makes the code more fragile, eg. when changing the code. Relying on SVG tags eg. circle is not as good because, in the future, we may always replace a terminal node with a g and there goes some selection that might have relied on tags.

Member

monfera commented Nov 2, 2017

@etpinard crossfilter mostly operates via the regular API, eg. Plotly.addTracesfor adding a salient trace that shows the selected points (will likely switch to the persistent selection for this), but not always. For example, adding a salient trace will duplicate the colorbar, but the original one shouldn't be shown. It is possible to Plotly.restyle the original trace to make the colorbar go away, and another Plotly.restyle to make it come back. These calls are expensive and the dashboard is quite slow as it is. To meet functional needs at decent speed, there are some shortcuts, mostly, setting the opacity of something to zero and back. These calls are infinitely fast, just one selection.style.

While this relies on implementation detail - the scenegraph - , it's unexposed and internal to the crossfilter code ie. we can trivially remove these once there is a fast, single-call API (such as Plotly.react as you suggest) that can take care of all changes with no duplicated overhead.

In the process, I saw a missing classname and thought it's good practice to add a class name anyway (and extract out the class names). As a matter of convention I automatically add class names to scenegraph elements because not doing so might introduce shallow, avoidable gotchas during dev, and makes the code more fragile, eg. when changing the code. Relying on SVG tags eg. circle is not as good because, in the future, we may always replace a terminal node with a g and there goes some selection that might have relied on tags.

@etpinard

This comment has been minimized.

Show comment
Hide comment
@etpinard

etpinard Nov 2, 2017

Member

💃

Member

etpinard commented Nov 2, 2017

💃

@monfera monfera merged commit f49b18e into master Nov 2, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@monfera monfera deleted the colorbar-class branch Nov 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment