feat: remove duplicate tick labels from axis #577

rshen91 · 2020-03-03T18:34:22Z

Summary

Closes #445 and addresses related kibana issue elastic/kibana#47045

BREAKING CHANGE:
showDuplicateTicks is set to false by default - this changes current behavior as seen in elastic/kibana#47045

This PR removes duplicate tickLabels for an axis instead of taking into account unique tickValues. For instance, if the tickLabel on the axis is showing days, then millisecond differences in the tickValues should not generate duplicate tickLabels. However, if the consumer wants to show duplicate tickLabels, then this can be configured with the showDuplicateTicks config prop but isn't set to true by default.

There has been a story added within Axes - Axes - Duplicate Ticks

Checklist

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

~~- [] Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)~~

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 · 2020-03-04T15:10:00Z

Codecov Report

Merging #577 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #577   +/-   ##
=======================================
  Coverage   70.55%   70.55%           
=======================================
  Files         220      220           
  Lines        6412     6412           
  Branches     1230     1230           
=======================================
  Hits         4524     4524           
  Misses       1869     1869           
  Partials       19       19

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 506bf0b...506bf0b. Read the comment docs.

markov00

I've added few comments that needs to be addressed before merging. Feature wise works fine

markov00 · 2020-03-10T18:09:30Z

src/chart_types/xy_chart/utils/specs.ts

@@ -525,11 +525,14 @@ export interface AxisSpec extends Spec {
  style?: AxisStyle;
  /** Show only integar values **/
  integersOnly?: boolean;
+  /** Remove duplicate ticks, default is false*/
+  duplicateTicks?: boolean;


@rshen91 Maybe we should introduce a breaking change and enabling this by default changing the name to enableDuplicatedTicks or something similar. What do you think?
@nickofthyme ideas?

I like the naming a lot better - please see commit 5037110 for naming changes

markov00 · 2020-03-10T18:14:30Z

stories/line/10_duplicate_ticks.tsx

+
+export const example = () => {
+  const now = DateTime.fromISO('2019-01-11T00:00:00.000')
+    .setZone('utc+1')


if you are using a timezone here, you should also add it to the LineSeries if not, the ticks will be slightly off

<LineSeries ... timeZone="utc+1"

Weirdly, utc+1 wasn't aligning but timeZone="local" seems to do the trick. Not really sure how that is possible. Commit ead2e9e for reference

markov00 · 2020-03-11T15:30:49Z

src/scales/scale_continuous.ts

@@ -139,6 +139,8 @@ interface ScaleOptions {
  isSingleValueHistogram: boolean;
  /** Show only integer values **/
  integersOnly?: boolean;
+  /** Show duplicate tick values default to false */
+  duplicateTicks?: boolean;


Why we need this in the scale option? the getDuplicateTicks doesn't use that but use directly the AxisSpec props

Ah thanks for spotting this, I didn't remove this properly after enabling the feature - I'm removing this in commit 5037110

markov00 · 2020-03-11T15:39:41Z

src/chart_types/xy_chart/utils/axis_utils.ts

+  if (axisSpec.duplicateTicks === false) {
+    const uniqueTickLabels: { value: any; label: string; position: number }[] = [];
+    allTicks.filter((value, index) => {
+      for (let i = 0; i < labels.length; i++) {
+        if (labels[i] === allTicks[index].label) {
+          uniqueTickLabels.push(allTicks[index]);
+          // once uniqueTickLabels has the unique label, remove the label from the labels array so it doesnt create a duplicate
+          labels.splice(i, 1);
+        }
+      }
+    });
+    return uniqueTickLabels;
+  } else {
+    return allTicks;
+  }


I think this code can be simplified a bit, let's chat on that directly

Thanks a ton for your help with me on this. I added commit 594c6f6 which I think gets done what we discussed, but lmk what you think!

nickofthyme · 2020-03-11T20:49:24Z

src/chart_types/xy_chart/utils/axis_utils.test.ts

+    const now = DateTime.fromISO('2019-01-11T00:00:00.000')
+      .setZone('utc+1')
+      .toMillis();
+    const oneDay = 1000 * 60 * 60 * 24;


you should leverage packages to your advantage. Like using duration from moment.

Suggested change

const oneDay = 1000 * 60 * 60 * 24;

const oneDay = moment.duration(1, 'day');

Cool! Pls see commit 55bf6ee for changes 💪

nickofthyme · 2020-03-11T20:54:09Z

src/chart_types/xy_chart/utils/axis_utils.test.ts

+      .setZone('utc+1')
+      .toMillis();
+    const oneDay = 1000 * 60 * 60 * 24;
+    const formatter = niceTimeFormatter([now, now + oneDay * 31]);


Better yet you could use moment's super easy and declarative API for this using .add and .subtract depending on what type of date niceTimeFormatter is expecting. If you need the value in mills you could use the duration.add.

Changes in commit 55bf6ee thanks!

nickofthyme · 2020-03-11T20:58:31Z

src/chart_types/xy_chart/utils/axis_utils.ts

+  return enableDuplicatedTicks(axisSpec, scale, offset, tickFormatOptions);
+}
+
+export function enableDuplicatedTicks(


nit: enableX to me sounds like you are toggling a config option or something of that nature. Also, your config property and this method have the same name, these should usually be different.

Thanks for pointing this out - changed the config property naming in commit
6ba6751

nickofthyme · 2020-03-11T21:00:56Z

src/chart_types/xy_chart/utils/specs.ts

@@ -525,11 +525,14 @@ export interface AxisSpec extends Spec {
  style?: AxisStyle;
  /** Show only integar values **/
  integersOnly?: boolean;
+  /** Remove duplicate ticks, default is false*/
+  enableDuplicatedTicks?: boolean;


These properties are more like the current state of something. Say showGridLines as in a boolean, it wouldn't be enableGirdLines because it is not performing an action like that would imply.

rshen91 · 2020-03-18T13:53:40Z

jenkins, test this please

markov00

LGTM

# [18.1.0](v18.0.0...v18.1.0) (2020-03-18) ### Bug Fixes * add unicorn eslint as dev dependency ([#591](#591)) ([30fd07c](30fd07c)) ### Features * remove duplicate tick labels from axis ([#577](#577)) ([e8c89ec](e8c89ec)), closes [#445](#445) * **api:** cleanup exposed types ([#593](#593)) ([544b7cc](544b7cc)) * **partition:** general sunburst via slice show control ([#592](#592)) ([5e6a30b](5e6a30b))

markov00 · 2020-03-18T22:04:58Z

🎉 This PR is included in version 18.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

markov00 · 2020-03-26T16:48:00Z

🎉 This PR is included in version 18.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

rshen91 force-pushed the duplicate-ticks branch from 697a832 to f4904f2 Compare March 3, 2020 18:36

rshen91 marked this pull request as ready for review March 4, 2020 16:18

rshen91 added wip work in progress bug Something isn't working labels Mar 4, 2020

rshen91 force-pushed the duplicate-ticks branch from a012f68 to 6408b59 Compare March 5, 2020 23:17

rshen91 added 5 commits March 5, 2020 15:33

fix: add helper function to remove duplicate tickLabels from axis

f784b2b

test: set utc time for test

e71ac18

refactor: clean up removeDupeTickLabels

f4eafa5

refactor: add filter after formatting

f7a1aa0

feat: adding duplicate ticks story

a0c8839

rshen91 force-pushed the duplicate-ticks branch from 6408b59 to a0c8839 Compare March 5, 2020 23:38

rshen91 added 9 commits March 6, 2020 04:50

test: add missing snapshot

d6fb645

Merge remote-tracking branch 'upstream/master' into duplicate-ticks

f263fbf

fix: add license heading to story file

d5abfd7

docs: correct x axis format for duplicates

591c603

test: update snapshot for x axis

dc9a38f

Merge remote-tracking branch 'upstream/master' into duplicate-ticks

efce9bf

feat: implement duplicate tick functionality

60664b9

Merge remote-tracking branch 'upstream/master' into duplicate-ticks

af7c506

style: clean up comments

935e64a

rshen91 removed the wip work in progress label Mar 10, 2020

rshen91 requested a review from markov00 March 10, 2020 17:46

markov00 requested a review from nickofthyme March 10, 2020 18:01

rshen91 added 2 commits March 11, 2020 09:18

test: add unit tests

4976be7

test: set timezone to clean up tests

1b76c69

markov00 requested changes Mar 11, 2020

View reviewed changes

rshen91 added 3 commits March 11, 2020 11:57

refactor: update naming and remove from scales spec

5037110

Merge remote-tracking branch 'upstream/master' into duplicate-ticks

1d888f7

refactor: add timeZone prop to story

ead2e9e

rshen91 added 2 commits March 12, 2020 08:55

refactor: improve filter() with a set and return allTicks earlier

c5ac664

refactor: refactor to reduce() implementation

4e88447

nickofthyme reviewed Mar 16, 2020

View reviewed changes

rshen91 added 6 commits March 17, 2020 07:37

Merge remote-tracking branch 'upstream/master' into duplicate-ticks

054c088

refactor: use moment library for time calculations

55bf6ee

refactor: change config prop name to show vs enable

6ba6751

fix: fix moment in duplicate ticks

9aa1329

refactor: revert moment changes in brush stories

4952e6b

Merge remote-tracking branch 'upstream/master' into duplicate-ticks

8a9739c

rshen91 requested a review from nickofthyme March 17, 2020 19:14

feat: refactor getUniqueValues to be reusable

594c6f6

rshen91 requested a review from markov00 March 17, 2020 20:33

rshen91 added 4 commits March 18, 2020 11:43

Merge remote-tracking branch 'upstream/master' into duplicate-ticks

a63b9eb

refactor: refactored generics in getUniqueValues

9ed79f0

style: remove export from computeTickDimensions

506bf0b

test: move story to axes stories and update vrt

d0ff3c8

markov00 approved these changes Mar 18, 2020

View reviewed changes

markov00 changed the title ~~fix: remove duplicate tick labels from axis~~ feat: remove duplicate tick labels from axis Mar 18, 2020

rshen91 merged commit e8c89ec into elastic:master Mar 18, 2020

rshen91 deleted the duplicate-ticks branch March 18, 2020 20:25

markov00 added the released Issue released publicly label Mar 18, 2020

markov00 mentioned this pull request Mar 18, 2020

Update dependency @elastic/charts to v18.1.0 elastic/kibana#60578

Merged

2 tasks

markov00 added the released on @18.1.x label Mar 26, 2020

Dosant mentioned this pull request May 6, 2020

TSVB - remove duplica x-axis legend entries when using interval >= 1d elastic/kibana#47045

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove duplicate tick labels from axis #577

feat: remove duplicate tick labels from axis #577

rshen91 commented Mar 3, 2020 •

edited

Loading

codecov-io commented Mar 4, 2020 •

edited

Loading

markov00 left a comment

markov00 Mar 10, 2020

rshen91 Mar 11, 2020 •

edited

Loading

markov00 Mar 10, 2020

rshen91 Mar 11, 2020 •

edited

Loading

markov00 Mar 11, 2020

rshen91 Mar 11, 2020

markov00 Mar 11, 2020

rshen91 Mar 17, 2020

nickofthyme Mar 11, 2020

rshen91 Mar 17, 2020

nickofthyme Mar 11, 2020

rshen91 Mar 17, 2020

nickofthyme Mar 11, 2020

rshen91 Mar 17, 2020

nickofthyme Mar 11, 2020

rshen91 commented Mar 18, 2020

markov00 left a comment

markov00 commented Mar 18, 2020

markov00 commented Mar 26, 2020

	const oneDay = 1000 * 60 * 60 * 24;
	const oneDay = moment.duration(1, 'day');

feat: remove duplicate tick labels from axis #577

feat: remove duplicate tick labels from axis #577

Conversation

rshen91 commented Mar 3, 2020 • edited Loading

Summary

Checklist

codecov-io commented Mar 4, 2020 • edited Loading

Codecov Report

markov00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshen91 Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshen91 Mar 11, 2020 • 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

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

rshen91 commented Mar 18, 2020

markov00 left a comment

Choose a reason for hiding this comment

markov00 commented Mar 18, 2020

markov00 commented Mar 26, 2020

rshen91 commented Mar 3, 2020 •

edited

Loading

codecov-io commented Mar 4, 2020 •

edited

Loading

rshen91 Mar 11, 2020 •

edited

Loading

rshen91 Mar 11, 2020 •

edited

Loading