feat(xy_charts): render legend inside the chart#1031
feat(xy_charts): render legend inside the chart#1031markov00 merged 16 commits intoelastic:masterfrom
Conversation
|
@miukimiu can you please double check from the UX prospective if we need to add a background layer below the legend and/or change the current highlight background on each legend item on hover, you can play with that on storybook at: http://localhost:9001/?path=/story/legend--inside-chart thanks |
Codecov Report
@@ Coverage Diff @@
## master #1031 +/- ##
==========================================
+ Coverage 71.55% 71.98% +0.42%
==========================================
Files 381 399 +18
Lines 11914 12271 +357
Branches 2588 2646 +58
==========================================
+ Hits 8525 8833 +308
- Misses 3358 3399 +41
- Partials 31 39 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@monfera @rshen91 @nickofthyme I'd like your opinion on the API as written on the PR description |
f857055 to
21fa501
Compare
elizabetdev
left a comment
There was a problem hiding this comment.
I've been experimenting on https://github.com/miukimiu/elastic-charts/tree/2021_01_21-legend_inside_chart how the legend would look like without the light gray background.
On hover, we could make the font-weight bolder and the color darker. But with a bolder font-weight, the text would sometimes shift a little bit. To avoid that we could use a text-shadow. But after experimenting with this solution, I came to the conclusion that:
- It's hard to notice the hover state in dark themes
So I think the light gray background works better for most scenarios.
Conclusion, LGTM! 🎉
I think it might make sense to have an internalLegendPosition property in addition to the legendPosition property to avoid confusion. |
There was a problem hiding this comment.
Sorry this took so long to get to.
I think most of this api is fine but I really don't like the tuple approach. I listed the negatives below:
- IMHO I think it is more complicated for the chart consumer to interoperate how to position the legend
- None of the positions mention the legend being internal, just shows positions (e.g.
['top', 'left']) - It creates more checking internally to determine if the position is internal and handling the
legendPositionas a tuple or aPositiontype top/bottommust come first thenleft/right
I think the best approach is to add these to the Position enum or likely another LegendPosition to avoid collisions.
const LegendPosition = Object.freeze({
...Position,
InsideTopLeft: 'ITL',
InsideTopRight: 'ITR',
InsideBottomLeft: 'IBL',
InsideBottomRight: 'IBR',
});simple API, explicit internal designation, cleaner internal values checks.
|
Is it a full understanding of the distinct envisioned use cases?
A single enum has the benefit of one, primitive type for the user (no union type), at the slight cost of bundling independent properties (into not too many, easy to understand, most frequently used options). Where it runs into limit is if we want to add pretty much anything; eg.
Addressing the last two are larger exercises that needs more planning anyway. So either way looks good if the table is a correct summary of the ~1year outlook, maybe enums are simpler (more than 7-9 enums and they become unmanageable) and involve a smaller API change; we could rethink for all these other use cases in a next round. But if we need relative coordinates now, or |
|
Some specifications don't combine, eg.
Maybe it's expected though, left for a future PR
|
| const totalVerticalPadding = padding * 2; | ||
| let legendHeight = 0; | ||
| if (showLegend && isHorizontalLegend(legendPosition)) { | ||
| if (showLegend && isHorizontalLegend(Array.isArray(legendPosition) ? legendPosition[1] : legendPosition)) { |
There was a problem hiding this comment.
If legendPosition can be an array, where is it specified? On a cursory search I only see
legendPosition: Position | LegendPositionConfig
There was a problem hiding this comment.
Oh thanks, good catch, it was the previous implementation, Let me fix this
There was a problem hiding this comment.
Weird that TS doesn't complain here that it narrowed to never and the test will be always falsey; structural typing without structural support 😄
| item: LegendItem; | ||
| totalItems: number; | ||
| position: Position; | ||
| legendPositionConfig: LegendPositionConfig; |
There was a problem hiding this comment.
As we are already inside LegendItemProps, and the type is prefixed with Legend, maybe the name could shorten to PositionConfig; the other props aren't prefixed with legend either
| return position; | ||
| } | ||
| return LEGEND_TO_FULL_CONFIG[position]; | ||
| } |
There was a problem hiding this comment.
Or just
return typeof position === 'object' ? position : LEGEND_TO_FULL_CONFIG[position];
There was a problem hiding this comment.
fixed in 95d59c9 (not sure why this doesn't shows up here as outdated)
src/components/legend/style_utils.ts
Outdated
| export function getLegendStyle(position: Position, size: BBox, margin: number): LegendStyle { | ||
| if (position === Position.Left || position === Position.Right) { | ||
| export function getLegendStyle(direction: LegendPositionConfig['direction'], size: BBox, margin: number): LegendStyle { | ||
| if (direction === 'vertical') { |
There was a problem hiding this comment.
Minor: here and elsewhere, using the "enum" feels a bit more reified:
if (direction === LayoutDirection.Vertical)
There was a problem hiding this comment.
thanks I'm gonna change every missing enums
| return { | ||
| flatLegend, | ||
| legendAction, | ||
| legendColorPicker, |
There was a problem hiding this comment.
This extraction of the listed legend props here wouldn't be needed if the legends spec props belonged in its cohesive object, via a single composite prop in SettingsSpec. As is, every future prop change/addition needs maintenance here too, so a bit less DRY
There was a problem hiding this comment.
I know, hope to get to this soon
| textOffsetY: number; | ||
| align: Extract<HorizontalAlignment, 'left' | 'center' | 'right'>; | ||
| verticalAlign: Extract<VerticalAlignment, 'top' | 'middle' | 'bottom'>; | ||
| align: Extract< |
There was a problem hiding this comment.
Is it intentionally just align rather than horizontalAlign?
There was a problem hiding this comment.
I've changed only the type declaration here, I can change the propname to reflect it better
monfera
left a comment
There was a problem hiding this comment.
Works well for me for the inside placed Cartesians. Thanks for assessing the individual comments. Unless there's concern about some of the options not yet functional (see comment with screenshot) it's good to go, it adds useful functionality and doesn't remove existing capabilities
nickofthyme
left a comment
There was a problem hiding this comment.
lol was just about to approve! 🟢
Verified with rotations on chrome. One thing we might want to add in the future is a theme item to set the box-shadow of the text and color to avoid hidden dots.
|
🎉 This PR is included in version 26.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [26.1.0](elastic/elastic-charts@v26.0.0...v26.1.0) (2021-03-26) ### Features * **a11y:** add basic aria-label to canvas element ([opensearch-project#1084](elastic/elastic-charts#1084)) ([d4b3e4f](elastic/elastic-charts@d4b3e4f)) * **xy_charts:** render legend inside the chart ([opensearch-project#1031](elastic/elastic-charts#1031)) ([b271d09](elastic/elastic-charts@b271d09)), closes [opensearch-project#861](elastic/elastic-charts#861)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1031 +/- ##
==========================================
+ Coverage 71.55% 71.98% +0.42%
==========================================
Files 381 399 +18
Lines 11914 12271 +357
Branches 2588 2646 +58
==========================================
+ Hits 8525 8833 +308
- Misses 3358 3399 +41
- Partials 31 39 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
This PR adds the ability to display the legend inside the XY cartesian charts.
The
<Settings>propertylegendPositionwill take now an extended version of the position that allow an increased customizability:The legend outside the chart only works with the following cases:
This in the future can be expanded to consider:
VerticalAlignment.MiddleHorizontalAlignment.Centerfix #861