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

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 Outdated 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 commented May 5, 2021 •

edited

Loading

markov00 left a comment

nickofthyme commented May 25, 2021

monfera May 31, 2021

markov00 May 31, 2021

nickofthyme May 31, 2021

monfera left a comment •

edited

Loading

nickofthyme commented May 31, 2021

markov00 left a comment

markov00 commented Jun 4, 2021

nickofthyme commented Jun 4, 2021

nickofthyme commented Jun 4, 2021

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

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

Conversation

nickofthyme commented May 5, 2021 • edited Loading

Summary

Details

Connected issues

Checklist

markov00 left a comment

Choose a reason for hiding this comment

nickofthyme commented May 25, 2021

monfera May 31, 2021

Choose a reason for hiding this comment

markov00 May 31, 2021

Choose a reason for hiding this comment

nickofthyme May 31, 2021

Choose a reason for hiding this comment

monfera left a comment • edited Loading

Choose a reason for hiding this comment

nickofthyme commented May 31, 2021

markov00 left a comment

Choose a reason for hiding this comment

markov00 commented Jun 4, 2021

nickofthyme commented Jun 4, 2021

nickofthyme commented Jun 4, 2021

nickofthyme commented May 5, 2021 •

edited

Loading

monfera left a comment •

edited

Loading