feat(legend): multiline labels with maxLines option by nickofthyme · Pull Request #1285 · elastic/elastic-charts

nickofthyme · 2021-08-04T22:47:36Z

Summary

The labelOptions.maxLines property is now available in on Theme.legend type to set the max number of lines a legend label can take-up before it is truncated.

Details

When enabled the multiline mode will trigger items to be align from the top (i.e. flex-start). These style changes have no affect on existing single line legend labels.

Screen.Recording.2021-08-05.at.11.43.31.AM.mp4

Issues

#1245

Checklist

The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
The proper feature label was added (e.g. :interactions, :axis) if the PR involves a specific chart feature
Whenever possible, please check if the closing issue is connected to a running GH project
Any consumer-facing exports were added to packages/charts/src/index.ts (and stories only import from ../src except for test data & storybook)
This was checked for cross-browser compatibility
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

nickofthyme · 2021-08-05T14:19:04Z

packages/charts/src/components/legend/_legend_item.scss

    display: flex;
    justify-content: center;
    align-items: center;
+    height: $legendItemHeight;


makes the focus size of the action the same as the color dot

nickofthyme · 2021-08-05T14:19:40Z

packages/charts/src/components/legend/label.tsx

      type="button"
      className={labelClassNames}
-      title={label}
+      title={options?.multiline ? '' : label} // full text already visible


I don't think the title is necessary if the full label is visible

nickofthyme · 2021-08-05T14:51:46Z

Improves inside legends ability to adapt to small widths. The inside legend styles, still needs some help (not in this pr).

monfera

Looks great so far! Should we breakup long words? Or maybe there's some upper limit to row count at play here:

nickofthyme · 2021-08-05T16:21:12Z

Should we breakup long words?

We could could add another option for overflow-wrap

Or maybe there's some upper limit to row count at play here

We could add a maxLines option like flutter does. I'm not sure if that's needed for the first pass, what do you think?

monfera · 2021-08-05T16:21:19Z

packages/charts/api/charts.api.md


+// @public (undocumented)
+export interface LegendLabelOptions {
+    multiline: boolean;


In partition chart, not for the legend, we use a maxRowCount property, which might be useful here and be a slightly more uniform API. So, a value of 1 would be the default, and it could be whatever number incl. Infinity if unbounded. While it's not integral to the task, it feels useful to set some sensible row count, and with a maxRowCount there's no need to break the API in the future, or introduce an additional property, which is only applicable with a multiline: true value. If a row count is deemed useful, we can preemptively cut down on ifs, ternaries and union types 😸

Yeah so you are thinking change multiline option to maxRowCount?

Done here d8be979

monfera · 2021-08-05T16:24:52Z

packages/charts/src/components/legend/legend_item.tsx

  positionConfig: LegendPositionConfig;
  extraValues: Map<string, LegendItemExtraValues>;
  showExtra: boolean;
+  labelOptions?: LegendLabelOptions;


Nit: could it be relatively easily solved such that it's only the user facing config API where this is optional, but we can rely on its presence everywhere downstream? Ie. one question mark only

You saying labelOptions: LegendLabelOptions;?

Yes, it's an internal type, by this time the reification (augmentation with defaults etc.) of the user input should've happened (if we have shared buy-in of how it often simplifies the implementation and prevents us from possibly handling the non-specified nullish case at multiple places, so also a DRY principle)

Done here d8be979

monfera · 2021-08-05T16:32:16Z

packages/charts/src/components/legend/label.tsx

  isSeriesHidden?: boolean;
  isToggleable?: boolean;
  onClick?: MouseEventHandler;
+  options?: LegendLabelOptions;


Minor: same here, maybe all such optionality like can go away except for the single public API enty point. I admit to having done plenty of this in code I wrote, more recently thinking that it'd be more economical to reify user input right away, and then avoid having to worry about partials, optionals etc.

We can think about removing question marks from non-@public types, TS is great for safely allowing such a transition, in the meantime we can consider avoiding them in new code

Done here d8be979

…ltiline-legend-labels

nickofthyme · 2021-08-05T16:38:38Z

integration/tests/legend_stories.test.ts

+    const getPositionalUrl = (p1: string, p2: string, others: string = '') =>
+      `http://localhost:9001/?path=/story/legend--inside-chart&knob-vAlign_Legend=${p1}&knob-hAlign_Legend=${p2}${others}`;


fixes knob to align with correct vAlign and hAlign properties.

monfera

Good improvement 🎉 Approving it as there's no blocker.

Optional notes:

if possible, tweak the CSS such that what's hidden now in case of very long, unbroken words would show up, but no need to invest too much time into it, as the user provided formatter function can detect and apply delimiters eg. at every 10 characters (so the user can solve cornercase need)
prefer using maxRowCount as it's numerical and reuses a name with the like meaning instead of the boolean, even if we initially support 1 and some other value eg. Infinity (if CSS reasonably supports a row count, we can use it now or in some future PR, or JS if really needed in the future)
prefer nullish coalescing of new API properties right at the entrance, so that the entire implementation has a fixed type to work with

nickofthyme · 2021-08-05T22:32:42Z

Replaced multiline option with maxLines option in d8be979.

0 - true multiline, unconstrained.
1 - as is today simple text-overflow, nowrap, etc.
>1 - limit lines of text to a finite value.

Screen.Recording.2021-08-05.at.05.28.28.PM.mp4

nickofthyme · 2021-08-06T15:23:19Z

integration/tests/interactions.test.ts

        { left: 282, top: 80 },
        {
          screenshotSelector: '#story-root',
+          delay: 1000,


This story was misbehaving, need to look into more later.

nickofthyme · 2021-08-06T15:24:10Z

packages/charts/src/components/legend/_legend_item.scss

  flex-wrap: nowrap;
  justify-content: space-between;
-  align-items: center;
+  align-items: flex-start;


This caused a slight diff in the inside legends but I think it's acceptable.

nickofthyme · 2021-08-06T15:25:36Z

packages/charts/src/components/legend/label.tsx

+    // This div is required to allow multiline text truncation, all ARIA requirements are still met
+    // https://stackoverflow.com/questions/68673034/webkit-line-clamp-does-not-apply-to-buttons
+    <div
+      role="button"
+      tabIndex={0}
      className={labelClassNames}
-      title={label}
+      title={title}
      onClick={onClick}
+      onKeyDown={onKeyDown}
+      aria-pressed={isSeriesHidden}


Converted to button due to this, added all required ARIA props to make it seem like a button.

nickofthyme · 2021-08-06T15:27:22Z

packages/charts/src/components/legend/legend_item.tsx

+          <div className="colorWrapper">
+            <ItemColor
+              ref={this.colorRef}
+              color={color}
+              seriesName={label}
+              isSeriesHidden={isSeriesHidden}
+              hasColorPicker={hasColorPicker}
+              onClick={this.handleColorClick(hasColorPicker)}
+              pointStyle={pointStyle}
+            />
+          </div>


This is to prevent jitter when using EuiWrappingPopover to display the color picker. Before this was picked using :first-of-type but this is more strict though a little more verbose.

nickofthyme · 2021-08-06T15:31:18Z

Linting job hung up for 30 minutes for some reason, not cancelable. Ran locally with no errors so gonna merge.

# [33.2.0](v33.1.0...v33.2.0) (2021-08-06) ### Bug Fixes * heatmap snap domain to interval ([#1253](#1253)) ([b439182](b439182)), closes [#1165](#1165) * hex colors to allow alpha channel ([#1274](#1274)) ([03b4f42](03b4f42)) ### Features * **bullet:** the tooltip shows up around the drawn part of the chart only ([#1278](#1278)) ([a96cbb4](a96cbb4)) * **legend:** multiline labels with maxLines option ([#1285](#1285)) ([e0eb096](e0eb096))

feat(legend): multiline labels

aff29d4

nickofthyme added enhancement New feature or request :legend Legend related issue :styling Styling related issue :all Applies to all chart types labels Aug 4, 2021

nickofthyme requested review from ghudgins, markov00, monfera and rshen91 August 4, 2021 22:47

ghudgins mentioned this pull request Aug 5, 2021

Better support for long values elastic/kibana#41418

Closed

nickofthyme commented Aug 5, 2021

View reviewed changes

nickofthyme added 2 commits August 5, 2021 09:22

fix: api changes

477f611

test: update jest snapshots

b60160f

nickofthyme marked this pull request as ready for review August 5, 2021 14:42

Merge branch 'master' into multiline-legend-labels

5b2bc29

monfera reviewed Aug 5, 2021

View reviewed changes

test: add vrts for new multiline option

bec8643

monfera reviewed Aug 5, 2021

View reviewed changes

nickofthyme added 3 commits August 5, 2021 11:32

Merge remote-tracking branch 'origin' into multiline-legend-labels

1859545

Merge remote-tracking branch 'origin' into multiline-legend-labels

d6fb516

Merge remote-tracking branch 'origin/multiline-legend-labels' into mu…

fcd45ca

…ltiline-legend-labels

nickofthyme commented Aug 5, 2021

View reviewed changes

nickofthyme linked an issue Aug 5, 2021 that may be closed by this pull request

[Legend] Improved readability on long legend values #1245

Closed

3 tasks

fix: align to top per marcos comments

88ebac5

monfera self-requested a review August 5, 2021 17:06

monfera approved these changes Aug 5, 2021

View reviewed changes

nickofthyme added 2 commits August 5, 2021 12:24

fix: labels with long words, now hyphenated

bdee07e

refactor: use max line prop instead of multiline boolean, add vrts

d8be979

nickofthyme added 5 commits August 5, 2021 17:34

test: update snapshots

6800bc2

test: update vrt screenshots

5959a18

fix: api changes

575b6da

fix: test and linting issues

697f73d

fix: attempted fix of flaky vrt

f796ae3

nickofthyme commented Aug 6, 2021

View reviewed changes

nickofthyme merged commit e0eb096 into elastic:master Aug 6, 2021

nickofthyme deleted the multiline-legend-labels branch August 6, 2021 15:31

nickofthyme changed the title ~~feat(legend): multiline labels~~ feat(legend): multiline labels with maxLines option Aug 6, 2021

		const getPositionalUrl = (p1: string, p2: string, others: string = '') =>
		`http://localhost:9001/?path=/story/legend--inside-chart&knob-vAlign_Legend=${p1}&knob-hAlign_Legend=${p2}${others}`;

Conversation

nickofthyme commented Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Issues

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickofthyme commented Aug 5, 2021

Uh oh!

monfera left a comment

Choose a reason for hiding this comment

Uh oh!

nickofthyme commented Aug 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfera left a comment

Choose a reason for hiding this comment

Uh oh!

nickofthyme commented Aug 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickofthyme commented Aug 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nickofthyme commented Aug 4, 2021 •

edited

Loading