Skip to content
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

Fix #1213 - improve render warning behavior #1249

Merged
merged 1 commit into from
Dec 16, 2020
Merged

Conversation

sc1f
Copy link
Contributor

@sc1f sc1f commented Nov 7, 2020

This PR fixes #1213 by improving the behavior of perspective-viewer's render warnings, which display when the chart is attempting to render a dataset that is larger than specified bounds and might have performance implications.

As outlined in #1213, render warnings would keep popping up even after a user has dismissed them, and it becomes an annoyance for the user. Additionally, the presence of the "Close" button allows there to be a state where the user has closed the warning that a subset of the data is being displayed, but if the viewer's config is not changed then there are no other indications that the user is looking at a subset of the data and not the whole.

This PR changes the behavior of the render warning in the following ways:

  • Render warnings are now persistent per-plugin: after a user clicks "Render all points", the warning will not appear again for that plugin, even after the user switches to a different plugin.
  • The "Close" button has been removed, so that there is always a warning present when the user is looking at a subset of data.
  • A new render_warning option has been added to each plugin config, which allows the user to specify whether the warning should be shown at all (defaults to true):
const plugin = window.getPlugin("d3_heatmap");

// Change the bounds at which the warning should appear
plugin.max_cells = 10000;
plugin.max_columns = 200;

// Or disable the warning entirely
plugin.render_warning = false
  • A new set of tests around the render warning has been added to several chart types in perspective-viewer-d3fc.

@sc1f sc1f added enhancement Feature requests or improvements JS 0.6.0 labels Nov 7, 2020
@sc1f sc1f added this to the 0.6.0 milestone Nov 7, 2020
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I think this implementation is an improvement over whats on master currently, a few things stick out to me re: the UX itself, that I'd like to answer in a future PR:

  • The X ("dismiss the warning") button does indeed remove any indication the visualization is compromised, but the actual current implementation crops the top 30px of the view. I can buy not wanting this to go away, but if its going to be persistent I think it needs a redesign - maybe integration with the upcoming status bar?
  • Is render_warning keyword a more obvious implementation of this feature than, e.g., max_cells: Infinity ? In essence, max_cells is a conceit to Perspective's somewhat asymmetric performance w.r.t. columns vs rows, so I can imagine a future simplification of this feature that only exports max_cells. At minimum, render_warning and max_* are exclusive and might benefit from some validation (and maybe also default to true 💯 ). Obviously, as this is a developer facing API, we have some room to iterate here!

@texodus texodus merged commit fd69a5c into master Dec 16, 2020
@texodus texodus deleted the fix-render-warning branch December 16, 2020 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests or improvements JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the behavior of the render warning on charts
2 participants