feat(axis): add tickLabelPadding prop #217

rshen91 · 2019-05-22T15:46:44Z

Summary

Closes #94
This is a duplicate of the closed PR #176 (closed in error)

Allows users to specify a tick label padding for charts. In storybook, when debug is checked, the user can add a specified tick label padding number to the bottom axis and rotations with ordinal axis.
In storybook this can be seen in the following charts:

Axis basic
Axis tick label rotation

This functionality is also included as a theme for the bottom tick label padding:

In the Stylings stories, there is a tickLabelPadding both prop and theme story included to show both the axis spec and theme manipulating tickLabelPadding.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] 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

codecov-io · 2019-05-22T16:29:21Z

Codecov Report

Merging #217 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
+ Coverage   97.76%   97.77%   +0.01%     
==========================================
  Files          36       36              
  Lines        2636     2654      +18     
  Branches      588      612      +24     
==========================================
+ Hits         2577     2595      +18     
  Misses         52       52              
  Partials        7        7

Impacted Files	Coverage Δ
src/lib/themes/light_theme.ts	`100% <ø> (ø)`	⬆️
src/lib/series/specs.ts	`100% <ø> (ø)`	⬆️
src/lib/themes/dark_theme.ts	`100% <ø> (ø)`	⬆️
src/lib/series/rendering.ts	`97.91% <100%> (+0.02%)`	⬆️
src/lib/axes/axis_utils.ts	`100% <100%> (ø)`	⬆️
src/lib/axes/canvas_text_bbox_calculator.ts	`100% <100%> (ø)`	⬆️
src/lib/utils/commons.ts	`100% <0%> (ø)`	⬆️

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 fc6640d...725177f. Read the comment docs.

rshen91 · 2019-05-22T17:42:56Z

Copying from earlier PR

Have you tested to see what happens if there is an axis-specific padding configuration versus if it's on the theme itself? May be useful to have a story showing the difference (see the custom series styling stories for an example).

added commit 617d8695

markov00 · 2019-05-22T20:41:08Z

hey @rshen91 is good to review or still a WIP?

rshen91 · 2019-05-22T21:50:41Z

hey @rshen91 is good to review or still a WIP?

Sorry about that @markov00! Please feel free to review when you have a chance

emmacunningham

left a few more comments and a question for discussion

src/components/react_canvas/axis.tsx

stories/axis.tsx

stories/styling.tsx

src/lib/series/specs.ts

src/lib/series/rendering.ts

emmacunningham · 2019-05-22T22:24:51Z

src/lib/axes/canvas_text_bbox_calculator.ts

@@ -17,7 +17,7 @@ export class CanvasTextBBoxCalculator implements BBoxCalculator {
    this.attachedRoot.appendChild(this.offscreenCanvas);
    this.scaledFontSize = scaledFontSize;
  }
-  compute(text: string, fontSize = 16, fontFamily = 'Arial', padding: number = 1): Option<BBox> {
+  compute(text: string, padding: number, fontSize = 16, fontFamily = 'Arial'): Option<BBox> {


@markov00 What do you think about doing some validation to prevent negative padding and if the supplied padding argumen is less than 1, set the padding to 1 instead? And we could also surface this in our configuration warnings once we have those. We are already accounting for padding in the computation of the label and if the user specifies a negative padding, then the text box would not be wide enough for the label.

Yes I'm ok to add some validation here to prevent negative padding.

Added commit edb1f4b to address

test added in 1507d9c to address

Just to confirm - should we set the padding to 1 when it's specified by the user at 0? Or should the validation only catch negative values? Thanks!

Clarified in zoom with Emma about this 👌

hardcoded in axis.tsx marked optional in specs.ts feat(axis_utils axis axis_utils.test): tickLabelPadding in axis_utils

refactor(added testUserInput number in knob): wip broken build

docs(axis): add storybook knob to tickLabelPadding prop feat(axis): add bottom axis tick label padding prop feat(rotations): add tickLabelPadding prop feat(bar_chart): add tickLabelPadding prop

feat(axis_utils): specify tickLabelPadding from axisSpec or axisConfig feat(bbox_calc axis_utils): remove optional padding for tickLabelPadding

refactor(axis_utils rendering): change parameter order and whitespace docs(chart_state): remove testUserInput test prop

style(stories/axis.tsx): remove explicit renderer for axis basic refactor(bar_chart rotations stories): remove tickLabelPadding

feat(specs.ts stories/axis.tsx): included tickLabelPadding prop hardcoded in axis.tsx marked optional in specs.ts style(react_canvas/axis): improve comment for padding 0

style(specs.ts): improve comment for tickLabelPadding spec

markov00

Everything seems to work correctly. I've just add a comment to address before merge.
I'm still seeing various indentation errors, but we will fix this moving from tslint + prettier to eslint + prettier soon.

src/lib/themes/light_theme.ts

markov00 · 2019-06-06T12:42:36Z

jenkins test this please

markov00

LGTM! 🚀

markov00 · 2019-06-10T17:02:37Z

@emmacunningham @rshen91 what do you think if we extend this PR to support custom axis styles?
because what is the meaning of having only the padding as a separate spec property here?
What if we extend this to have something that accept custom style for axis like:

<Axis
  id={getAxisId('bottom')}	        
  position={Position.Bottom}	        
  title={'Bottom axis'}	          
  showOverlappingTicks={true}
  style={{
    tickLabelStyle: {
      padding: 10
    }
  }}
/>

nickofthyme · 2019-06-24T20:56:37Z

src/lib/axes/canvas_text_bbox_calculator.ts

    if (!this.context) {
      return none;
    }

+    // Avoid having negative padding that can obscure text


I'd say generally comments as simple as this can be left for tests. Meaning you should create test cases that validate this line of code. That way if someone removes these lines of code, the tests break.

Not saying you can't leave comments, but do so for more complex code that requires explanation.

Ok! I think I wrote one that gets to the issue in commit 1507d9c. Let me know what you think thanks!

FYI Github will auto link and format the commit hash when you just put the hash or short hash like this 1507d9c but when you wrap the hash in backticks, or anything else for that matter, it doesn't create the link like this 1507d9c. Here is a link to all the url fun https://help.github.com/en/articles/autolinked-references-and-urls

You can also paste code snippets by selecting code lines, use shift+click for multiple lines, and select copy permalink

Then you will get something like this...

elastic-charts/package.json

Lines 14 to 16 in f2b90df

"scripts": {

"cz": "git-cz",

"build:clean": "rm -rf ./dist",

Just FYI - It took me a while to realize how many helpful tools GH has for our use.

improved comment in commit d8dd8f4

src/lib/axes/axis_utils.test.ts

nickofthyme

LGTM, tested locally.

emmacunningham · 2019-07-01T07:29:48Z

@rshen91 let's chat on zoom about how to do this.

@emmacunningham @rshen91 what do you think if we extend this PR to support custom axis styles?
because what is the meaning of having only the padding as a separate spec property here?
What if we extend this to have something that accept custom style for axis like:
<Axis
  id={getAxisId('bottom')}	        
  position={Position.Bottom}	        
  title={'Bottom axis'}	          
  showOverlappingTicks={true}
  style={{
    tickLabelStyle: {
      padding: 10
    }
  }}
/>	 

Commit 776239a to address

emmacunningham · 2019-07-03T06:59:23Z

src/lib/axes/axis_utils.ts

+  const tickLabelPadding =
+    axisSpec.style && axisSpec.style.tickLabelPadding
+      ? axisSpec.style.tickLabelPadding
+      : axisConfig.tickLabelStyle.padding;


Can we rename this parameter name to themeAxisConfig to make it clearer where this is coming from? I think eventually we'll want to think about how to better merge spec-specific and theme-general styles but for now, would be nice to update the parameter name.

Thank you for helping me with this! The commit with the changes in naming and improving axis_utils test is addressed in commit d8dd8f4

markov00

@rshen91 I've added two comments, the one of the if check needs to be addressed before merging.
the other can be done also on a different PR if we want

src/lib/axes/axis_utils.ts

src/lib/series/specs.ts

markov00

LGTM! 🚀

# [7.1.0](v7.0.2...v7.1.0) (2019-07-03) ### Features * **axis:** add tickLabelPadding prop ([#217](#217)) ([4d40936](4d40936)), closes [#94](#94)

markov00 · 2019-07-03T19:25:14Z

🎉 This PR is included in version 7.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [7.1.0](elastic/elastic-charts@v7.0.2...v7.1.0) (2019-07-03) ### Features * **axis:** add tickLabelPadding prop ([opensearch-project#217](elastic/elastic-charts#217)) ([080df95](elastic/elastic-charts@080df95)), closes [opensearch-project#94](elastic/elastic-charts#94)

rshen91 force-pushed the rshen-94 branch from 6f90073 to abcc959 Compare May 22, 2019 17:04

rshen91 mentioned this pull request May 22, 2019

add tickLabelPadding prop #176

Closed

2 tasks

rshen91 requested a review from markov00 May 22, 2019 20:53

emmacunningham reviewed May 22, 2019

View reviewed changes

rshen91 added 9 commits May 23, 2019 08:10

feat(specs.ts stories/axis.tsx): included tickLabelPadding prop

f3032fe

hardcoded in axis.tsx marked optional in specs.ts feat(axis_utils axis axis_utils.test): tickLabelPadding in axis_utils

feat(axis.tsx axis_utils.ts): tickLabelPadding extend width of Text Axis

be36cb1

refactor(added testUserInput number in knob): wip broken build

feat(axis): add tickLabelPadding prop

451ecfb

docs(axis): add storybook knob to tickLabelPadding prop feat(axis): add bottom axis tick label padding prop feat(rotations): add tickLabelPadding prop feat(bar_chart): add tickLabelPadding prop

feat(styling and axis): supress canvas padding manipulation

c092953

feat(axis_utils): specify tickLabelPadding from axisSpec or axisConfig feat(bbox_calc axis_utils): remove optional padding for tickLabelPadding

feat(axis_utils canvas_bbox): add theme tickLabelPadding

ab3869a

refactor(axis_utils rendering): change parameter order and whitespace docs(chart_state): remove testUserInput test prop

style(rendering.ts and test): define padding as default to 1

9bfa9b1

style(axis.tsx): improve comment line 32

0a3f443

style(stories/axis.tsx): remove explicit renderer for axis basic refactor(bar_chart rotations stories): remove tickLabelPadding

refactor(stories/axis): add tickLabelPadding to tick label rotation axes

b5dee48

feat(specs.ts stories/axis.tsx): included tickLabelPadding prop hardcoded in axis.tsx marked optional in specs.ts style(react_canvas/axis): improve comment for padding 0

feat(axis styling.tsx): add tickLabelPadding story theme and prop

2dfb614

rshen91 force-pushed the rshen-94 branch from 617d869 to 2dfb614 Compare May 23, 2019 14:14

rshen91 added 2 commits May 23, 2019 08:33

style(axis.ts rendering.ts): min to -90 readability and comments

929b0ac

refactor(styling): isolate spec and theme in tickLabelPadding story

0f39133

style(specs.ts): improve comment for tickLabelPadding spec

markov00 changed the title ~~add tickLabelPadding prop~~ feat: add tickLabelPadding prop May 31, 2019

markov00 added :chart Chart element related issue enhancement New feature or request labels May 31, 2019

markov00 reviewed Jun 3, 2019

View reviewed changes

src/lib/themes/light_theme.ts Show resolved Hide resolved

rshen91 added 2 commits June 4, 2019 08:49

refactor(canvas_bbox dark_theme): add validation for padding

edb1f4b

Merge remote-tracking branch 'upstream/master' into rshen-94

83ee9b7

rshen91 requested review from emmacunningham and markov00 June 6, 2019 14:38

emmacunningham changed the title ~~feat: add tickLabelPadding prop~~ feat(axis): add tickLabelPadding prop Jun 6, 2019

emmacunningham removed the :chart Chart element related issue label Jun 6, 2019

markov00 approved these changes Jun 6, 2019

View reviewed changes

Merge branch 'master' into pr/217

2dab12a

markov00 self-requested a review June 10, 2019 17:03

rshen91 added 2 commits June 24, 2019 08:47

refactor: merge commit

7bba2fa

refactor: missed adding merge commits

f8e8f5f

nickofthyme reviewed Jun 24, 2019

View reviewed changes

test(axis_utils.test.ts): add test to confirm positive padding

1507d9c

nickofthyme reviewed Jun 25, 2019

View reviewed changes

src/lib/axes/axis_utils.test.ts Show resolved Hide resolved

nickofthyme approved these changes Jun 25, 2019

View reviewed changes

rshen91 added 2 commits July 1, 2019 11:21

Merge remote-tracking branch 'upstream/master' into rshen-94

fac9f17

refactor(axis_utils specs): create style object

776239a

emmacunningham reviewed Jul 3, 2019

View reviewed changes

test(axis_utils.test): add test for tickLabelPadding style prop or theme

d8dd8f4

rshen91 requested a review from emmacunningham July 3, 2019 15:04

markov00 reviewed Jul 3, 2019

View reviewed changes

src/lib/axes/axis_utils.ts Outdated Show resolved Hide resolved

src/lib/series/specs.ts Show resolved Hide resolved

refactor(axis_utils): refactor getAxisTickLabelPadding

83fcc7d

rshen91 requested a review from markov00 July 3, 2019 17:11

refactor(axis_utils): improve if statement

725177f

rshen91 mentioned this pull request Jul 3, 2019

Refactor AxisStyle object to include custom styles for AxisSpec #260

Closed

3 tasks

markov00 approved these changes Jul 3, 2019

View reviewed changes

rshen91 merged commit 4d40936 into elastic:master Jul 3, 2019

markov00 pushed a commit that referenced this pull request Jul 3, 2019

chore(release): 7.1.0 [skip ci]

3c291a2

# [7.1.0](v7.0.2...v7.1.0) (2019-07-03) ### Features * **axis:** add tickLabelPadding prop ([#217](#217)) ([4d40936](4d40936)), closes [#94](#94)

markov00 added the released Issue released publicly label Jul 3, 2019

rshen91 deleted the rshen-94 branch July 3, 2019 19:37

markov00 mentioned this pull request Jul 10, 2019

Upgrade @elastic/charts dependency to 7.2.1 elastic/kibana#40751

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(axis): add tickLabelPadding prop #217

feat(axis): add tickLabelPadding prop #217

rshen91 commented May 22, 2019 •

edited

Loading

codecov-io commented May 22, 2019 •

edited

Loading

rshen91 commented May 22, 2019 •

edited

Loading

markov00 commented May 22, 2019

rshen91 commented May 22, 2019

emmacunningham left a comment

emmacunningham May 22, 2019

markov00 Jun 3, 2019

rshen91 Jun 4, 2019

rshen91 Jun 25, 2019

rshen91 Jun 25, 2019

rshen91 Jul 3, 2019

markov00 left a comment

markov00 commented Jun 6, 2019

markov00 left a comment •

edited

Loading

markov00 commented Jun 10, 2019 •

edited

Loading

nickofthyme Jun 24, 2019

rshen91 Jun 25, 2019

nickofthyme Jun 25, 2019

rshen91 Jul 3, 2019

nickofthyme left a comment

emmacunningham commented Jul 1, 2019 •

edited by rshen91

Loading

emmacunningham Jul 3, 2019

rshen91 Jul 3, 2019

markov00 left a comment

markov00 left a comment

markov00 commented Jul 3, 2019

feat(axis): add tickLabelPadding prop #217

feat(axis): add tickLabelPadding prop #217

Conversation

rshen91 commented May 22, 2019 • edited Loading

Summary

Checklist

codecov-io commented May 22, 2019 • edited Loading

Codecov Report

rshen91 commented May 22, 2019 • edited Loading

markov00 commented May 22, 2019

rshen91 commented May 22, 2019

emmacunningham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

markov00 commented Jun 6, 2019

markov00 left a comment • edited Loading

Choose a reason for hiding this comment

markov00 commented Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickofthyme left a comment

Choose a reason for hiding this comment

emmacunningham commented Jul 1, 2019 • edited by rshen91 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

markov00 commented Jul 3, 2019

rshen91 commented May 22, 2019 •

edited

Loading

codecov-io commented May 22, 2019 •

edited

Loading

rshen91 commented May 22, 2019 •

edited

Loading

markov00 left a comment •

edited

Loading

markov00 commented Jun 10, 2019 •

edited

Loading

emmacunningham commented Jul 1, 2019 •

edited by rshen91

Loading