add tickLabelPadding prop #176

rshen91 · 2019-04-17T22:00:29Z

Summary

Closes #94

Allows users to specify a debug tick label padding for charts. 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:

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

markov00 · 2019-04-18T12:54:48Z

@rshen91 thanks for the work you are doing here.
@emmacunningham do we want to have this as a props for the axis or as a theme configuration?

emmacunningham · 2019-04-18T14:23:38Z

@markov00 Ah good point. Perhaps it can be that there's a default padding amount in the theme and if the user wants a specific padding amount for just one axis, then they can use the axis prop to specify it for just one axis?

codecov-io · 2019-04-18T15:21:30Z

Codecov Report

Merging #176 into master will increase coverage by 0.4%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #176     +/-   ##
=========================================
+ Coverage    97.4%   97.81%   +0.4%     
=========================================
  Files          35       35             
  Lines        2120     2560    +440     
  Branches      315      558    +243     
=========================================
+ Hits         2065     2504    +439     
- Misses         48       49      +1     
  Partials        7        7

Impacted Files	Coverage Δ
src/lib/themes/light_theme.ts	`100% <ø> (ø)`	⬆️
src/lib/series/specs.ts	`100% <ø> (ø)`	⬆️
src/lib/series/rendering.ts	`97.84% <100%> (+0.36%)`	⬆️
src/lib/axes/axis_utils.ts	`100% <100%> (ø)`	⬆️
src/lib/axes/canvas_text_bbox_calculator.ts	`100% <100%> (ø)`	⬆️
src/state/crosshair_utils.ts	`84.12% <0%> (-0.19%)`	⬇️
src/specs/settings.tsx	`100% <0%> (ø)`	⬆️
src/state/annotation_utils.ts	`100% <0%> (ø)`	⬆️
src/lib/themes/theme.ts	`100% <0%> (ø)`	⬆️
src/lib/series/domains/y_domain.ts	`100% <0%> (ø)`	⬆️
... and 21 more

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 e38e8f6...b66f0b5. Read the comment docs.

emmacunningham · 2019-05-21T16:18:06Z

src/components/react_canvas/axis.tsx

@@ -29,7 +29,12 @@ export class Axis extends React.PureComponent<AxisProps> {
    return this.renderAxis();
  }
  renderTickLabel = (tick: AxisTick, i: number) => {
-    const { padding, ...labelStyle } = this.props.chartTheme.axes.tickLabelStyle;
+    // suppress padding render from canvas


Can we update this description to clarify why we are suppressing the padding here?

comment improved in commit c70edf4

thanks for doing that! it would be helpful to include the context that we're already taking padding into account and that the computed bbox dimensions have adjusted for the padding. for someone unfamiliar with the implementation of tick label padding, they would not know why we're just zero-ing out the configured padding setting here, so we want to explain to future developers that we're doing this because padding has already been accounted for and how.

src/state/chart_state.ts

stories/axis.tsx

stories/bar_chart.tsx

stories/rotations.tsx

emmacunningham · 2019-05-21T16:40:19Z

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


I wonder if we should do some validation on this argument since we (internally) want this number to be at least 1 or higher (we need to have at least 1 pixel of padding to help with Chrome's inconsistencies in measureText computations).

@markov00 what do think about doing some validation and if the padding amount is less than 1 we set the padding to a default of 1 (and when we start implementing misconfiguration warnings, add a warning if a user tries to specify a padding less than 1)?

emmacunningham · 2019-05-21T16:44:17Z

stories/axis.tsx

@@ -82,6 +84,7 @@ storiesOf('Axis', module)
    );
  })
  .add('tick label rotation', () => {
+    const tickLabelPadding = number('Bottom Axis Tick Label Padding', 0);


Can you add the padding for all of the axes in this story so users can see how the padding would affect each axis & the positioning of the label within the bounding box?

Should the 'w many tick labels' story have tickLabelPadding added to it?

added padding to all axes for tick label rotation story in this commit 60d95aa

Should the 'w many tick labels' story have tickLabelPadding added to it?

Yeah, the question that will help you make this decision: would adding a knob for tickLabelPadding in a specific story reveal or show people something that is different from what they can find in another story? In the case of w many tick labels, it might be worth showing how padding interacts with showOverlappingLabels.

emmacunningham · 2019-05-21T16:45:26Z

stories/styling.tsx

@@ -172,7 +172,7 @@ storiesOf('Stylings', module)
          fontSize: range('tickFontSize', 0, 40, 10, 'Tick Label'),
          fontFamily: `'Open Sans', Helvetica, Arial, sans-serif`,
          fontStyle: 'normal',
-          padding: 0,
+          padding: number('tickLabelPadding', 1, {}, 'Tick Label'),


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

story added in commit 617d869 in new pr

ghost · 2019-05-22T14:14:11Z

Hi @rshen91, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

rshen91 changed the title ~~tickLabelPadding prop~~ [WIP] add tickLabelPadding prop Apr 17, 2019

rshen91 force-pushed the rshen-94 branch from 6857cca to 6c49622 Compare April 18, 2019 15:17

emmacunningham added wip work in progress :axis Axis related issue labels Apr 18, 2019

rshen91 force-pushed the rshen-94 branch from 0064cbd to b005f5e Compare April 24, 2019 17:24

rshen91 force-pushed the rshen-94 branch from 97f8ea6 to df33042 Compare May 20, 2019 13:55

rshen91 requested a review from emmacunningham May 20, 2019 22:42

emmacunningham removed the wip work in progress label May 21, 2019

emmacunningham reviewed May 21, 2019

View reviewed changes

emmacunningham requested a review from markov00 May 21, 2019 17:05

rshen91 changed the title ~~[WIP] add tickLabelPadding prop~~ add tickLabelPadding prop May 21, 2019

rshen91 closed this May 22, 2019

rshen91 force-pushed the rshen-94 branch from b66f0b5 to 9b2ba6e Compare May 22, 2019 15:03

rshen91 mentioned this pull request May 22, 2019

feat(axis): add tickLabelPadding prop #217

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add tickLabelPadding prop #176

add tickLabelPadding prop #176

rshen91 commented Apr 17, 2019 •

edited

Loading

markov00 commented Apr 18, 2019

emmacunningham commented Apr 18, 2019

codecov-io commented Apr 18, 2019 •

edited

Loading

emmacunningham May 21, 2019

rshen91 May 21, 2019

emmacunningham May 21, 2019

emmacunningham May 21, 2019

emmacunningham May 21, 2019

rshen91 May 21, 2019

rshen91 May 21, 2019

emmacunningham May 21, 2019

emmacunningham May 21, 2019

rshen91 May 22, 2019

ghost commented May 22, 2019

add tickLabelPadding prop #176

add tickLabelPadding prop #176

Conversation

rshen91 commented Apr 17, 2019 • edited Loading

Summary

Checklist

markov00 commented Apr 18, 2019

emmacunningham commented Apr 18, 2019

codecov-io commented Apr 18, 2019 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 22, 2019

rshen91 commented Apr 17, 2019 •

edited

Loading

codecov-io commented Apr 18, 2019 •

edited

Loading