feat: auto legend resize #316

nickofthyme · 2019-08-15T03:16:30Z

Summary

Remove UI legend toggle
Allow legend content to determine size of legend.
Pull Legend out of Canvas space
Update styles for top/bottom legend to use css grid

The simple explanation of changes:

Changes to `Right` and `Left` legends

The old legend was statically positioned using a hardcoded width. This created a lot of useless white space for series with short labels and small values like this..............................👇👇👇👇👇👇

With these changes, the legendItems are computed and rendered, then measured to determine the width. The width includes the formatted series label plus the formatted last value.

Then a so-called "buffer" is added to this measured width value to account for the variability in value length (i.e. values ranging from 0 to 1000000000). This buffer is a new property on the Theme type theme.legend.spacingBuffer. This should be increased for datasets with large variability.

If the value plus label ever exceeds the allowable width, the label will be truncated as it does currently.

This measured width is capped at a max value set by the Theme value theme.legend.verticalWidth.

Changes to `Top` and `Bottom` legends

The previously hardcoded height is now set to be a max-height to reduce white space.

This new max-height value is determined from the Theme value theme.legend.horizontalHeight.

CSS flex was replaced with CSS grid to better adjust to window sizing. Previously the widths were hardcoded and if too long would just wrap to a new row. like this 👇

With CSS grid and repeat(auto-fill, [VALUE]) the columns grow and shrink to fill the empty space and wrap only when the min-width is not available like so 👇

Note: The CSS grid changes do not apply to IE11 because IE11 😩. IE will still use css flex.

Empty State

As the legend is now a separate containing element, the chart will grow to fill only the available space, rather than before having to compute and position each accordingly. As a result, the empty state is now centered relative to the available chart element, see below 👇

Demos

Left/Right

Top/Bottom

Checklist

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
This was checked for cross-browser compatibility, including a check against IE11
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios
Each commit follows the convention

Remove UI legend toggle. showLegend prop on settings unchanged. elastic#268

nickofthyme · 2019-08-15T03:27:50Z

@markov00 The toggle removal was pretty trivial but sizing the legend was quite tricky and this is the best solution I could come up with.

I had to observe the legendPosition value to update the Chart clases. I pulled the legend out from the echContainer so that the legend can take up the room it needs then the container can take up the room left available for rendering the canvas elements. I don't really like that logic required in chart.tsx so I am open to ideas.

Another thing I had to change was that the legendItems are computed in the computeChart method but I need the legend items for the parent dimensions to be set. I'm basically breaking on the first time when the legend is initialized to rerun the computations.

Lastly the Right and Left legends now having a not-static width will cause the chart to twitch when the values in the legend start to change. A quick fix to this was just to set a min-width to prevent that, but it's only to a certain point. See gif below for what I'm referring to.

markov00 · 2019-08-16T07:43:53Z

hey @nickofthyme I don't like much the fact that the chart twitch when moving the mouse over, nor I don't like much when, for Top and Bottom legend the legend twitch itself in the same way.
Also the wired connection between store - container - legend to specify the flex nature of the chart container is a bit hacky IMHO.

What if we try a different way? so before we used to have a fixed width/height. What if we can compute that dimension? It doesn't have to be pixel perfect, but enough to nicely fit on the screen:

we can get the longest data series name +
we can also compute an hypothetical max value to be displayed on the legend (hypothetical because depends on the formatter, and a value of 1.000.000 can be shortest than 100.000 if we use unit formatter like 1M 100K)
we use these two values to compute an average legend item fixed width. everything else should shrink in that max width.

This is not an optimal approach but can work.

A feature that we can include is also a user defined size for the legend.

src/specs/specs_parser.tsx

codecov-io · 2019-08-20T14:32:49Z

Codecov Report

Merging #316 into master will increase coverage by 0.49%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
+ Coverage   98.05%   98.55%   +0.49%     
==========================================
  Files          38       38              
  Lines        2726     2899     +173     
  Branches      643      692      +49     
==========================================
+ Hits         2673     2857     +184     
+ Misses         48       39       -9     
+ Partials        5        3       -2

Impacted Files	Coverage Δ
src/utils/themes/dark_theme.ts	`100% <ø> (ø)`	⬆️
src/utils/themes/theme.ts	`100% <ø> (ø)`	⬆️
src/utils/themes/light_theme.ts	`100% <ø> (ø)`	⬆️
src/specs/settings.tsx	`96.51% <100%> (+0.04%)`	⬆️
src/chart_types/xy_chart/utils/dimensions.ts	`100% <100%> (+20%)`	⬆️
src/specs/specs_parser.tsx	`100% <100%> (ø)`	⬆️
src/chart_types/xy_chart/utils/axis_utils.ts	`100% <100%> (ø)`	⬆️
src/chart_types/xy_chart/store/chart_state.ts	`97.12% <88.88%> (+0.23%)`	⬆️
src/chart_types/xy_chart/utils/scales.ts	`100% <0%> (ø)`	⬆️
src/chart_types/xy_chart/utils/specs.ts	`100% <0%> (ø)`	⬆️
... and 2 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 cf00fd9...8b9c663. Read the comment docs.

Refactor css flex styles to use grid for more shinny UI!

markov00

Code LGTM, I've added a minor comment.

I've tested locally using the playground on chrome, IE11 and Firefox (everything is fine).
On Safari, when showing a Bottom/Top legend the the content of the legend doesn't scroll and overflow the chart (usually it's just some different default/required css style:

markov00 · 2019-08-21T10:28:22Z

src/components/chart.tsx

    return (
      <Provider chartStore={this.chartSpecStore}>
-        <Fragment>
+        <div style={containerStyle} className={chartClassNames}>
+          <Legend legendId={this.legendId} />


As you removed the legend button, I thin you can also remove entirely the legendId

Oh good idea, I wasn't sure what that was for.

src/chart_types/xy_chart/store/chart_state.ts

Remove legendId, fix safari max-height inheritance issue, remove uneeded legendList styles, revert empty state flex value.

nickofthyme · 2019-08-21T14:54:22Z

@markov00 good catch, sorry I forgot about safari 🙃.

The problem was that we are setting a max-height on the Legend then the scrolling LegendListContainer had a height of 100% that safari was not obeying. So putting the max-height on the scroll container fixed it, looks good in all browsers now.

Safari

nickofthyme · 2019-08-21T15:23:29Z

@markov00 the flex: 1 1 auto caused this to happen only in safari 👇 Where the No data to display text is at the top not the center.

markov00

LGTM

# [10.0.0](v9.2.1...v10.0.0) (2019-08-21) ### Bug Fixes * **tooltip:** ie11 flex sizing ([#334](#334)) ([abaa472](abaa472)), closes [#332](#332) * decuple brush cursor from chart rendering ([#331](#331)) ([789f85a](789f85a)), closes [elastic/kibana#36517](elastic/kibana#36517) * remove clippings from chart geometries ([#320](#320)) ([ed6d0e5](ed6d0e5)), closes [#20](#20) ### Features * auto legend resize ([#316](#316)) ([659d27e](659d27e)), closes [#268](#268) * customize number of axis ticks ([#319](#319)) ([2b838d7](2b838d7)) * **theme:** base theme prop ([#333](#333)) ([a9ff5e1](a9ff5e1)), closes [#292](#292) ### BREAKING CHANGES * **theme:** remove `baseThemeType` prop on `Settings` component and `BaseThemeTypes` type. * `theme.legend.spacingBuffer` added to `Theme` type. Controls the width buffer between the legend label and value.

markov00 · 2019-08-21T19:58:40Z

🎉 This PR is included in version 10.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

nickofthyme added 2 commits August 14, 2019 13:12

feat(legend): remove ui legend toggle

b59fb13

Remove UI legend toggle. showLegend prop on settings unchanged. elastic#268

fix(legend): allow content to set size

e409eb2

nickofthyme requested a review from markov00 August 15, 2019 03:16

nickofthyme added 2 commits August 19, 2019 08:43

refactor: measure vert legend width with static buffer

750206b

fix: legend toggle issue for top/bottom legends

ad6232c

nickofthyme commented Aug 19, 2019

View reviewed changes

src/specs/specs_parser.tsx Show resolved Hide resolved

nickofthyme marked this pull request as ready for review August 19, 2019 13:50

nickofthyme added 3 commits August 19, 2019 09:34

Merge branch 'master' into fix/legend-spacing

d383325

fix: merge errors

51041af

test: fix broken tests

981cf6b

nickofthyme requested a review from cchaos August 20, 2019 23:25

nickofthyme added 2 commits August 20, 2019 18:46

feat(legend): flex styles to grid

a335522

Refactor css flex styles to use grid for more shinny UI!

Merge branch 'master' into fix/legend-spacing

9fa3373

markov00 requested changes Aug 21, 2019

View reviewed changes

markov00 changed the title ~~Legend sizing and ui toggle~~ feat: auto legend resize Aug 21, 2019

fix: pr comments

ea01ba3

Remove legendId, fix safari max-height inheritance issue, remove uneeded legendList styles, revert empty state flex value.

chore: remove unused styles

8b9c663

markov00 approved these changes Aug 21, 2019

View reviewed changes

nickofthyme merged commit 659d27e into elastic:master Aug 21, 2019

nickofthyme deleted the fix/legend-spacing branch August 21, 2019 16:25

markov00 added the released Issue released publicly label Aug 21, 2019

nickofthyme mentioned this pull request Aug 22, 2019

Dynamic legend content size #311

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: auto legend resize #316

feat: auto legend resize #316

nickofthyme commented Aug 15, 2019 •

edited

Loading

nickofthyme commented Aug 15, 2019

markov00 commented Aug 16, 2019

codecov-io commented Aug 20, 2019 •

edited

Loading

markov00 left a comment

markov00 Aug 21, 2019

nickofthyme Aug 21, 2019

nickofthyme Aug 21, 2019

nickofthyme commented Aug 21, 2019

nickofthyme commented Aug 21, 2019

markov00 left a comment

markov00 commented Aug 21, 2019

feat: auto legend resize #316

feat: auto legend resize #316

Conversation

nickofthyme commented Aug 15, 2019 • edited Loading

Summary

Changes to Right and Left legends

Changes to Top and Bottom legends

Empty State

Demos

Left/Right

Top/Bottom

Checklist

nickofthyme commented Aug 15, 2019

markov00 commented Aug 16, 2019

codecov-io commented Aug 20, 2019 • edited Loading

Codecov Report

markov00 left a comment

Choose a reason for hiding this comment

markov00 Aug 21, 2019

Choose a reason for hiding this comment

nickofthyme Aug 21, 2019

Choose a reason for hiding this comment

nickofthyme Aug 21, 2019

Choose a reason for hiding this comment

nickofthyme commented Aug 21, 2019

Safari

nickofthyme commented Aug 21, 2019

markov00 left a comment

Choose a reason for hiding this comment

markov00 commented Aug 21, 2019

nickofthyme commented Aug 15, 2019 •

edited

Loading

Changes to `Right` and `Left` legends

Changes to `Top` and `Bottom` legends

codecov-io commented Aug 20, 2019 •

edited

Loading