feat(axis): allow pixel domain padding for y axes by nickofthyme · Pull Request #1145 · elastic/elastic-charts

nickofthyme · 2021-05-05T15:28:46Z

Summary

Adds support for pixel domain padding in the vertical/y axis, added to the existing padding options of domain value and domain ratio value.

BREAKING CHANGES:
domain.padding now only takes a number value. If you are using a percent-based padding such as '10%' please set domain.padding to 0.1 and domain.paddingUnit to DomainPaddingUnit.DomainRatio.

Details

This pr adds the ability to set the y domain padding to a fixed pixel value in the spatial or screen space. Currently, the domain is allowed to be set to a domain value or domain percentage value with the misaligned documentation.

Adds paddingUnit as type DomainPaddingUnit to domain options rather than parsing units from a string.

export const DomainPaddingUnit: Readonly<{
    Domain: "domain";
    Pixel: "pixel";
    DomainRatio: "domainRatio";
}>;

Adds domainPixelPadding and constrainDomainPadding as options to continuous scale, but does not apply to time scales.

Note: All domain padding is applied before and domain nicing.

Connected issues

Closes #1144

Checklist

Any consumer-facing exports were added to 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

- Add missing yarn call - Pass all yarn script args to jest call in vrt.sh

markov00

I'm a bit confused about when/if the padding is applied to the lower part of the domain.
It looks like it always gets applied to the higher part, and the lower one always gets limited at 0 or at the specified domain min.

scripts/vrt.sh

src/scales/scale_continuous.ts

nickofthyme · 2021-05-25T16:46:20Z

It looks like it always gets applied to the higher part, and the lower one always gets limited at 0 or at the specified domain min.

This is controlled by the constrainPadding option.

- Add new tests - Fix broken tests - Fix error in inverted domain padding logic

monfera · 2021-05-31T09:19:56Z

src/scales/scale_continuous.ts

+  const { scaleMultiplier } = screenspaceMarkerScaleCompressor(
+    orderedDomain,
+    [2 * desiredPixelPadding, 2 * desiredPixelPadding],
+    chartHeight,
+  );
+  let paddedDomainLo = orderedDomain[0] - desiredPixelPadding / scaleMultiplier;
+  let paddedDomainHigh = orderedDomain[1] + desiredPixelPadding / scaleMultiplier;
+
+  if (constrainDomainPadding) {
+    if (paddedDomainLo < 0 && orderedDomain[0] >= 0) {
+      const { scaleMultiplier } = screenspaceMarkerScaleCompressor(
+        [0, orderedDomain[1]],
+        [0, 2 * desiredPixelPadding],
+        chartHeight,
+      );
+      paddedDomainLo = 0;
+      paddedDomainHigh = orderedDomain[1] + desiredPixelPadding / scaleMultiplier;
+    } else if (paddedDomainHigh > 0 && orderedDomain[1] <= 0) {
+      const { scaleMultiplier } = screenspaceMarkerScaleCompressor(
+        [orderedDomain[0], 0],
+        [2 * desiredPixelPadding, 0],
+        chartHeight,
+      );
+      paddedDomainLo = orderedDomain[0] - desiredPixelPadding / scaleMultiplier;
+      paddedDomainHigh = 0;
+    }
+  }


Nice solution for adding both a top and bottom padding except when the padding would be the thing that extends the domain over/under the zero axis line 🎉

We might have a situation in the future where the reference line is not the zero line, but something else; for such a case, it would be eventually useful to DRY up and extract the reference value, eg. const intercept = 0; so it's later easy to make configurable. It's not needed in this PR, do you think @markov00 it'd be useful at some point?

Yep, is definitely a nice to have!

Is this what you had in mind @monfera 0577a2c?

monfera

Great PR Nick 🚀

Please add VRT pics for the full variety of cases, with top and bottom padding applied:

domain values are both positive and negative
positive-only and negative-only; both with and without the padding breaching the zero intercept

This also exercises all the code paths where the scale compressor is used

nickofthyme · 2021-05-31T13:36:16Z

Great PR Nick 🚀

Please add VRT pics for the full variety of cases, with top and bottom padding applied:

domain values are both positive and negative

positive-only and negative-only; both with and without the padding breaching the zero intercept

This also exercises all the code paths where the scale compressor is used

❤️ Good idea, see 7511666

markov00

LGTM, thanks Nick for all the changes

markov00 · 2021-06-04T15:13:12Z

jenkins test this please

nickofthyme · 2021-06-04T15:16:30Z

jenkins test this please

# [30.0.0](v29.2.0...v30.0.0) (2021-06-04) ### Bug Fixes * **domain:** custom domain should not filter data ([#1181](#1181)) ([76e8dca](76e8dca)), closes [#1129](#1129) * **value_labels:** zero as a valid value for textBorder and borderWidth ([#1182](#1182)) ([a64f333](a64f333)) * annotation tooltip display when remounting specs ([#1167](#1167)) ([8408600](8408600)) * render nodeLabel formatted text into the nodes ([#1173](#1173)) ([b44bdff](b44bdff)) ### Features * **axis:** allow pixel domain padding for y axes ([#1145](#1145)) ([7c1fa8e](7c1fa8e)) * apply value formatter to the default legend item label ([#1190](#1190)) ([71474a5](71474a5)) * **tooltip:** stickTo vertical middle of the cursor ([#1163](#1163)) ([380363b](380363b)), closes [#1108](#1108) * **wordcloud:** click and over events on text ([#1180](#1180)) ([196fb6a](196fb6a)), closes [#1156](#1156) ### BREAKING CHANGES * **value_labels:** the `textBorder` of `ValueFillDefinition` is now optional or a number only

nickofthyme · 2021-06-04T16:03:38Z

🎉 This PR is included in version 30.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

nickofthyme added 2 commits May 5, 2021 10:01

feat(axis): allow pixel domain padding for y axes

610eafc

ci: fix vrt config

00da2a0

- Add missing yarn call - Pass all yarn script args to jest call in vrt.sh

nickofthyme added enhancement New feature or request :axis Axis related issue breaking change labels May 5, 2021

nickofthyme requested a review from markov00 May 5, 2021 15:28

Merge branch 'master' into padding-px-value

4c036d7

markov00 reviewed May 14, 2021

View reviewed changes

scripts/vrt.sh Show resolved Hide resolved

src/scales/scale_continuous.ts Outdated Show resolved Hide resolved

refactor: use screenspaceMarkerScaleCompressor for domain logic

14c792b

nickofthyme added 7 commits May 25, 2021 11:46

Merge branch 'master' into padding-px-value

ef922e7

Merge branch 'master' into padding-px-value

5add7bb

test: add new tests, fix old tests

5d58b30

- Add new tests - Fix broken tests - Fix error in inverted domain padding logic

Merge branch 'master' into padding-px-value

55bfc12

fix: api comment spelling

4e273db

chore: update vrt screenshots

d2b50dc

Merge branch 'master' into padding-px-value

3da6c41

nickofthyme changed the title ~~feat(axis): allow pixel domain padding for y axes~~ feat(axis): allow pixel domain padding for y axes May 27, 2021

Merge branch 'master' into padding-px-value

c419f77

nickofthyme requested review from markov00 and monfera May 28, 2021 14:27

monfera reviewed May 31, 2021

View reviewed changes

nickofthyme mentioned this pull request May 31, 2021

Improved pixel padding logic #1179

Open

nickofthyme added 2 commits May 31, 2021 08:14

refactor: add intercept to padding logic

0577a2c

test: add vrt for domain cases

7511666

nickofthyme added 2 commits June 3, 2021 13:10

chore: add alpha tag to pixel unit

f90dc7e

Merge branch 'master' into padding-px-value

9b3e740

nickofthyme requested a review from monfera June 3, 2021 18:20

markov00 approved these changes Jun 4, 2021

View reviewed changes

Merge branch 'master' into padding-px-value

5690fab

nickofthyme enabled auto-merge (squash) June 4, 2021 13:08

nickofthyme merged commit 7c1fa8e into elastic:master Jun 4, 2021

nickofthyme deleted the padding-px-value branch June 4, 2021 15:52

nickofthyme added the released Issue released publicly label Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(axis): allow pixel domain padding for y axes #1145

feat(axis): allow pixel domain padding for y axes #1145
nickofthyme merged 17 commits intoelastic:masterfrom
nickofthyme:padding-px-value

nickofthyme commented May 5, 2021 •

edited

Loading

Uh oh!

markov00 left a comment

Uh oh!

Uh oh!

Uh oh!

nickofthyme commented May 25, 2021

Uh oh!

monfera May 31, 2021

Uh oh!

markov00 May 31, 2021

Uh oh!

nickofthyme May 31, 2021

Uh oh!

monfera left a comment •

edited

Loading

Uh oh!

nickofthyme commented May 31, 2021

Uh oh!

markov00 left a comment

Uh oh!

markov00 commented Jun 4, 2021

Uh oh!

nickofthyme commented Jun 4, 2021

Uh oh!

nickofthyme commented Jun 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

Summary

Details

Connected issues

Checklist

Uh oh!

markov00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nickofthyme commented May 25, 2021

Uh oh!

monfera May 31, 2021

Choose a reason for hiding this comment

Uh oh!

markov00 May 31, 2021

Choose a reason for hiding this comment

Uh oh!

nickofthyme May 31, 2021

Choose a reason for hiding this comment

Uh oh!

monfera left a comment • edited 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 commented May 31, 2021

Uh oh!

markov00 left a comment

Choose a reason for hiding this comment

Uh oh!

markov00 commented Jun 4, 2021

Uh oh!

nickofthyme commented Jun 4, 2021

Uh oh!

nickofthyme commented Jun 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nickofthyme commented May 5, 2021 •

edited

Loading

monfera left a comment •

edited

Loading