-
Notifications
You must be signed in to change notification settings - Fork 121
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
feat(partition): add legend and highlighters #616
Conversation
Codecov Report
@@ Coverage Diff @@
## master #616 +/- ##
=======================================
Coverage 66.81% 66.81%
=======================================
Files 252 252
Lines 7166 7166
Branches 1352 1352
=======================================
Hits 4788 4788
Misses 2361 2361
Partials 17 17 Continue to review full report at Codecov.
|
Based on the screenshots and gifs, this looks like exactly what we are looking for! Thank you! |
}); | ||
|
||
return ( | ||
<Chart className="story-chart" /*size={{ width: 1200, height: 800 }}*/> |
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.
You might want to remove this comment?
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.
Tested locally and stories seem to be rendering correctly and quickly on Chrome. There's a lot I like in this PR: moving LegendItem and seriesId into commons especially. I was a little thrown at first what legend files would remain specific for either xy_charts or partitions and so I'm going to include in here how I'm understanding the breakup of the legend (which to be clear, I think it makes a lot of sense). As far as I'm understanding it looks like the majority of the legend logic is in the components directory, LegendItem has been moved to the commons directory, and then the implementation of compute_legend
is specific for either partition charts or xy_charts. This makes a lot more sense to me setting it up this way. Any future types of charts then just need to have their own compute_legend.ts file. Let me know if I'm misunderstanding, thanks!
The highlighter.tsx file is very cool for computing the focusing on specific slices or not - I wonder if this might be able to also help in terms of keyboard navigation in the future (from what I understand working with SVG for keyboard navigation vs canvas is much easier)?
The new story in legend, single sunburst in the sunburst stories, and adding the <Settings showLegend />
in some sunburst stories, interactions, and two layers stress test in treemaps looks awesome! 💯
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.
Overall code changes look good. A few nits and comments. Much cleaner shared structure ❤️
Tested stories locally in Chrome and Firefox. All look good. Banded legend changes work as normal.
Tested triple-layer sunburst in IE11 with flatLegend
and legendMaxDepth
. Renders fine but seems to render a little slow still. Also get long-running script
warning but eventually renders without incident.
Unrelated: I noticed a small issue on the tooltip. Opened an issue here #628
src/chart_types/partition_chart/state/selectors/compute_legend.ts
Outdated
Show resolved
Hide resolved
src/chart_types/partition_chart/state/selectors/compute_legend.ts
Outdated
Show resolved
Hide resolved
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.
Epic PR with tons of legends rework to make it more general! Good to go, the singularity case can be a subsequent PR by either of us
jenkins test this |
# [18.3.0](v18.2.2...v18.3.0) (2020-04-15) ### Bug Fixes * remove series with undefined splitSeriesAccessor values ([#627](#627)) ([59f0f6e](59f0f6e)) ### Features * gauge, goal and bullet graph (alpha) ([#614](#614)) ([5669178](5669178)) * **partition:** add legend and highlighters ([#616](#616)) ([6a4247e](6a4247e)), closes [#486](#486) [#532](#532)
🎉 This PR is included in version 18.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [18.3.0](elastic/elastic-charts@v18.2.2...v18.3.0) (2020-04-15) ### Bug Fixes * remove series with undefined splitSeriesAccessor values ([opensearch-project#627](elastic/elastic-charts#627)) ([e8bd5ea](elastic/elastic-charts@e8bd5ea)) ### Features * gauge, goal and bullet graph (alpha) ([opensearch-project#614](elastic/elastic-charts#614)) ([4191664](elastic/elastic-charts@4191664)) * **partition:** add legend and highlighters ([opensearch-project#616](elastic/elastic-charts#616)) ([b939596](elastic/elastic-charts@b939596)), closes [opensearch-project#486](elastic/elastic-charts#486) [opensearch-project#532](elastic/elastic-charts#532)
Summary
This PR adds the legend to pie charts and treemaps.
Close #486 finishing the decoupling of the legend from XY charts
Close #532 request to add legend and tooltips to partition charts
Two more options are added to the
<Settings/>
componentsflatLegend
will flat the legend for pie/tree hierarchical legend as in the following screenshotlegendMaxDepth
will limit the legend hierarchy to a maximum depth value:1
is the root level. Can be combined with theflatLegend
legendMaxDepth={1}
example on a 3 level piechartlegendMaxDepth={2}
example on a 3 level piechartThe PR also add the highlighter when hovering over the pie/tree chart and over the legend items as shown in the following gifs
Checklist
Delete any items that are not applicable to this PR.
src/index.ts
(and stories only import from../src
except for test data & storybook)