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

Legend for deck.gl scatterplot #4572

Merged
merged 8 commits into from
Mar 14, 2018

Conversation

betodealmeida
Copy link
Member

This PR changes the deck.gl scatterplot to show a legend when there are multiple categories. Categories can be toggled by clicking on the legend:

deckgl_legend

@vylc, what should we do when there are too many values in the legend?

@codecov-io
Copy link

codecov-io commented Mar 8, 2018

Codecov Report

Merging #4572 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4572      +/-   ##
==========================================
- Coverage    71.2%   71.19%   -0.01%     
==========================================
  Files         187      189       +2     
  Lines       14785    14840      +55     
  Branches     1083     1086       +3     
==========================================
+ Hits        10527    10566      +39     
- Misses       4255     4271      +16     
  Partials        3        3
Impacted Files Coverage Δ
...rset/assets/javascripts/explore/stores/visTypes.js 70.58% <ø> (ø) ⬆️
...set/assets/javascripts/explore/stores/controls.jsx 38.16% <ø> (ø) ⬆️
superset/jinja_context.py 73.33% <0%> (-4.93%) ⬇️
superset/models/core.py 86.7% <0%> (-0.62%) ⬇️
...javascripts/SqlLab/components/SqlEditorLeftBar.jsx 92.45% <0%> (-0.55%) ⬇️
superset/sql_lab.py 74.21% <0%> (-0.13%) ⬇️
superset/utils.py 87.83% <0%> (-0.11%) ⬇️
superset/connectors/druid/models.py 76.31% <0%> (-0.04%) ⬇️
superset/assets/javascripts/explore/index.jsx 0% <0%> (ø) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ad50c9...5a16cf7. Read the comment docs.

toggleCategory: () => {},
};

export default class Legend extends React.PureComponent {
Copy link
Member

Choose a reason for hiding this comment

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

Can we name this MapLegend or do you seeing this being extended to all visualizations?

Copy link
Member Author

Choose a reason for hiding this comment

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

My idea was to use this as a legend for all visualization types that don't have a native one (like nvd3 has). Right now this component takes only a list of categories and is not specific to maps.

@betodealmeida
Copy link
Member Author

legend_position

@betodealmeida
Copy link
Member Author

overflow

@betodealmeida
Copy link
Member Author

Once this is merged I'll add support in the other deck.gl viz types.

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

A few minor things, otherwise LGTM

return null;
}

const categories = Object.entries(this.props.categories).map(([k, v]) => {
Copy link
Member

Choose a reason for hiding this comment

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

This may require a polyfill we may or may not have. Let's dbl-check whether it needs to be added.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool. We do have the module object.entries (which is a polyfill) included as a dependency of a dependency, I'll add it explicitly.

const icon = v.enabled ? '\u25CF' : '\u25CB';
return (
<li>
<a onClick={() => this.props.toggleCategory(k)}>
Copy link
Member

Choose a reason for hiding this comment

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

It's not needed, but it could be nice to select only current on dblclick, and would match the nvd3 behavior.

data = data.filter(inFrame);
if (filters != null) {
filters.forEach((f) => {
data = data.filter(f);
Copy link
Member

Choose a reason for hiding this comment

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

interesting pattern here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Functional JS! ;)

Maybe we should call this postFilters or something similar, since it's a series of filters applied after the query (due to clicks from the legend or the play slider).

@mistercrunch mistercrunch merged commit 7089344 into apache:master Mar 14, 2018
@mistercrunch mistercrunch deleted the DPTOOLS-326_deckgl_legend branch March 14, 2018 23:40
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
* Initial work

* Working version

* Specify legend position

* Max height with scroll

* Fix lint

* Better compatibility with nvd3

* Fix object.keys polyfill version

* Fix lint
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* Initial work

* Working version

* Specify legend position

* Max height with scroll

* Fix lint

* Better compatibility with nvd3

* Fix object.keys polyfill version

* Fix lint
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.24.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.24.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants