-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
toggleCategory: () => {}, | ||
}; | ||
|
||
export default class Legend extends React.PureComponent { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Once this is merged I'll add support in the other deck.gl viz types. |
There was a problem hiding this 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]) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)}> |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting pattern here :)
There was a problem hiding this comment.
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).
* Initial work * Working version * Specify legend position * Max height with scroll * Fix lint * Better compatibility with nvd3 * Fix object.keys polyfill version * Fix lint
* Initial work * Working version * Specify legend position * Max height with scroll * Fix lint * Better compatibility with nvd3 * Fix object.keys polyfill version * Fix lint
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:
@vylc, what should we do when there are too many values in the legend?