feat(xy): hide labels that protrude the bar geometry by markov00 · Pull Request #1233 · elastic/elastic-charts

markov00 · 2021-07-07T17:44:55Z

Summary

The overflowConstraints prop now replaces the existing BarSeries.displayValueSettings.hideClippedValue prop extending the existing feature with the ability to hide labels that protrude the bar geometry.

BREAKING CHANGE: an API change is introduced: hideClippedValue is removed in favour of overflowConstraints?: Array<LabelOverflowConstraint>;. The array can contain one or multiple overflow constraints identified in LabelOverflowConstraint

Details

I've added a different prop because both visibility filters are valid and useful.

Issues

fix #1234

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)
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

dej611

@dej611 Thanks for the review

the new API prop is not super nice.

The previous one was singular (*ClippingValue) while this has plural name *ClippingValues.

It's a long name but at the same time it has shortenings in it (*Geom*). I would rather have a full name >hideGeometryClippedValue.

As alternative I would consider also adding an object value to the hideClippedValue as:
hideClippedValue?: boolean | { edges?: boolean, geometries?: boolean }
The default boolean value should be converted to { edges?: boolean } internally

I've discussed and agree with @monfera to use a slight cleaner approach here 97fd23b
the hideClippedValue and hideGeomClippedValues are replaced by:

const LabelOverflowConstraint = Object.freeze({
  BarGeometry: 'barGeometry' as const,
  ChartEdges: 'chartEdges' as const,
});
...
hideIfOverflows?: Array<LabelOverflowConstraint>;

This allows us and the users to: combine easily the constraints. The API is also scalable and can accommodate other constraints along the way.

I find a bit confusing the way this flag works combined with the contain value label within bar element

I get that probably they won't get combined that often, but while playing in the storybook example I found that enabling >both leads to something I would not expect.

If there's a plan to remove the contain value label within bar element flag, it does not matter probably

You are right, I've deprecated the API prop and we will remove it soon.

Also, can you add the same flag to the advanced value labels example?

BREAKING CHANGE: the 'hideIfOverflows' option now replace 'hideClippedValues'

nickofthyme

LGTM, so if barGeometry is not specified in the hideIfOverflow prop, then this value below would be visible correct?

package.json

packages/charts/src/chart_types/xy_chart/rendering/bars.ts

packages/charts/api/charts.api.md

monfera · 2021-07-12T23:24:26Z

packages/charts/src/chart_types/xy_chart/rendering/bars.ts


-    const displayValue =
-      displayValueSettings && displayValueSettings.showValueLabel
+    const displayValue: BarGeometry['displayValue'] | undefined =


Does it need to also be undefined, or could it just be an empty string? There are then many ways to not render an empty string, eg. just render it (nothing will show up) or test for text lengt === 0 or test for text truthiness ("" is falsey)

this object has multiple properties, is not just the text string. IMHO I don't see how this can be better than having or not having an object.

monfera · 2021-07-12T23:28:03Z

packages/charts/src/chart_types/xy_chart/rendering/bars.ts

            height: textScalingFactor * fixedFontScale,
-            hideClippedValue,
-            isValueContainedInElement: displayValueSettings.isValueContainedInElement,
+            hideIfOverflowsChartEdges,


Let's suppose, there's yet another constraint, would it need a new parameter here? If it's likely, maybe it's easier to pass around the Set, and check for has at the place of application

fixed in 5f0b2ee

monfera · 2021-07-12T23:44:20Z

packages/charts/src/chart_types/xy_chart/rendering/bars.ts

+    const hideIfOverflows: Set<LabelOverflowConstraint> = new Set(
+      displayValueSettings?.hideIfOverflows ?? [
+        LabelOverflowConstraint.ChartEdges,
+        LabelOverflowConstraint.BarGeometry,
+      ],
+    );


Still pondering: the users of the library are not end users but coders, so a Set<LabelOverflowConstraint> might be a clearer API receptacle as the input is a set, rather than a bag (=multiset ie. possible duplicates) or array (ordered multiset). At the expense of trivial verbosity. From your earlier example:

overflowConstraints={['barGeometry', 'chartEdges']} overflowConstraints={new Set(['barGeometry', 'chartEdges'])}

As a consumer of the API I think the Set is an implementation detail that should not be exposed. Unless there's another API usage of the Set container I see no reason to expose it.

I agree with @dej611 here, adding a Set doesn't prevent the user adding duplicates in the array overflowConstraints={new Set(['barGeometry', 'chartEdges','barGeometry'])}
The cardinality is relatively low that it's easy to spot that manually. Internally we just get rid of duplicates

monfera

LGTM, the comments are just for consideration, some of them fitting in the old theme of trying to avoid/remove union types esp. where the non-nil type can accomodate a sensible "nil" value eg. NaN for numbers or '' for strings

markov00 · 2021-07-13T10:10:50Z

@nickofthyme

LGTM, so if barGeometry is not specified in the hideIfOverflow prop, then this value below would be visible correct?

Yes, you are right

nickofthyme

Updated changes LGTM!

# [33.0.0](v32.0.0...v33.0.0) (2021-07-14) ### Features * **xy:** add null bars to geometry indexing ([#1226](#1226)) ([20b81a9](20b81a9)), closes [#950](#950) * **xy:** hide labels that protrude the bar geometry ([#1233](#1233)) ([be1fb3d](be1fb3d)), closes [#1234](#1234) ### BREAKING CHANGES * **xy:** an API change is introduced: `hideClippedValue` is removed in favor of `overflowConstraints?: Array<LabelOverflowConstraint>;`. The array can contain one or multiple overflow constraints enumerated as `LabelOverflowConstraint`

nickofthyme · 2021-07-14T15:08:04Z

🎉 This PR is included in version 33.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

markov00 added 4 commits July 7, 2021 19:19

feat(xy): remove values that overflows the bars

dc896e8

test: update VRTS

4e33030

feat: add hideGeomClippedValues to clip only on geoms

8864d6d

Merge branch 'master' into 2021_07_07-fix_clipped_values

f14a6a6

markov00 added :annotation Annotation (line, rect, text) related issue :xy Bar/Line/Area chart related enhancement New feature or request labels Jul 8, 2021

markov00 marked this pull request as ready for review July 8, 2021 13:12

markov00 requested review from dej611, monfera, nickofthyme and rshen91 July 8, 2021 13:20

markov00 changed the title ~~feat(xy): fix clipped values~~ feat(xy): hide labels that protrude the bar geometry Jul 8, 2021

fix: realign debug with the panel transform

7473f81

dej611 reviewed Jul 8, 2021

View reviewed changes

markov00 added 3 commits July 12, 2021 14:32

refactor: better API naming

97fd23b

BREAKING CHANGE: the 'hideIfOverflows' option now replace 'hideClippedValues'

docs: update API

c18750f

test: update screenshot URL

8cb7c20

dej611 mentioned this pull request Jul 12, 2021

[Lens] Remove value labels flag for vertical displacement elastic/kibana#105231

Closed

9 tasks

nickofthyme approved these changes Jul 12, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

packages/charts/src/chart_types/xy_chart/rendering/bars.ts Show resolved Hide resolved

packages/charts/api/charts.api.md Outdated Show resolved Hide resolved

monfera reviewed Jul 12, 2021

View reviewed changes

monfera approved these changes Jul 12, 2021

View reviewed changes

markov00 added 3 commits July 13, 2021 11:58

refactor: enable scalable overflow constraint objects

5f0b2ee

ci: revert change on local server port

4d2a1c4

refactor: rename prop to overflowConstraints

c894328

markov00 requested review from monfera and nickofthyme July 13, 2021 10:11

markov00 requested a review from dej611 July 13, 2021 10:11

Merge branch 'master' into 2021_07_07-fix_clipped_values

4fc7fe6

nickofthyme approved these changes Jul 13, 2021

View reviewed changes

Merge branch 'master' into 2021_07_07-fix_clipped_values

dcc23b0

markov00 merged commit be1fb3d into elastic:master Jul 14, 2021

markov00 deleted the 2021_07_07-fix_clipped_values branch July 14, 2021 12:21

nickofthyme added the released Issue released publicly label Jul 14, 2021

This was referenced Jul 28, 2021

Value label is printed out with bad spacing in horizontal bar charts with isValueContainedInElement flag enabled #791

Closed

Zero values are printed on top of other values when in stacked histogram mode #790

Closed

Conversation

markov00 commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Issues

Checklist

Uh oh!

dej611 left a comment • edited by markov00 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

monfera Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

markov00 Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

monfera Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

markov00 Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

monfera Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

dej611 Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

markov00 Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

monfera left a comment

Choose a reason for hiding this comment

Uh oh!

markov00 commented Jul 13, 2021

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

nickofthyme commented Jul 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

markov00 commented Jul 7, 2021 •

edited

Loading

dej611 left a comment •

edited by markov00

Loading