feat(axis): option to hide duplicate axes #370

nickofthyme · 2019-09-10T22:33:32Z

Summary

Adds prop hideDuplicateAxes to settings to hide axes based on tick labels, position and title.

Refactor axes render function.

Axis will be hidden if all of the following conditions are true:

The first and last tick labels (with applied tickFormat) match
The length of the ticks match
The titles match, can be both undefined
The position on the spec match

closes #368

Checklist

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

* Option to hide axes based on tick labels, position and title. * Refactor axes render function. closes elastic#368

codecov-io · 2019-09-10T22:37:39Z

Codecov Report

❗ No coverage uploaded for pull request base (master@af4805f). Click here to learn what that means.
The diff coverage is 94.44%.

@@            Coverage Diff            @@
##             master     #370   +/-   ##
=========================================
  Coverage          ?   98.25%           
=========================================
  Files             ?       38           
  Lines             ?     2801           
  Branches          ?      661           
=========================================
  Hits              ?     2752           
  Misses            ?       44           
  Partials          ?        5

Impacted Files	Coverage Δ
src/specs/settings.tsx	`94.56% <100%> (ø)`
src/chart_types/xy_chart/store/chart_state.ts	`97.45% <93.75%> (ø)`

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 af4805f...2474776. Read the comment docs.

monfera · 2019-09-11T15:13:10Z

src/chart_types/xy_chart/store/chart_state.ts

+
+    let hasDuplicate = false;
+    tickMap.forEach(({ tickLabels: axisTickLabels }, axisId) => {
+      if (


Instead of a let hasDuplicate and forEach, let's use a tickmap.some(...)

Ya would totally agree unfortunately this is a Map not an Array which doesn't have the some prototype. 😞

Ah indeed! We could do tickMap.keys().some(...) tickMap.entries().some(...) but these would already visit all elements; a for or while loop feeding from an iterator could bail early but probably not worth the hassle.

monfera · 2019-09-11T15:20:51Z

src/chart_types/xy_chart/store/chart_state.ts

@@ -844,6 +845,35 @@ export class ChartStore {
    this.annotationSpecs.delete(annotationId);
  }

+  isDuplicateAxis(


On an initial look, this method isn't using this, it would be better to turn it into a regular function const isDuplicateAxis = ():({}) = {} completely outside anything that's to do with objects/classes. Even if such functions use this here and there, it'd be advantageous to represent logic as that; we can still pass this.foo or whatever upon invocation. This lets us lighten up our objects, make the logic reusable and refactorable (even into another file if needed), and help us reduce object orientation.

Sure I agree with that idea and we can make an effort to use that here and going forward, also in components.

monfera · 2019-09-11T15:32:16Z

src/components/react_canvas/reactive_chart.tsx

+    const ids = [...axesVisibleTicks.keys()];
+
+    return ids.reduce(
+      (acc, id) => {


I'd usually retain reduce for slightly more complex cases. Here it's essentially a ids.filter().map() except that you want to avoid the recalculation of ticks, axisSpec, axisTicksDimensions and axisPosition. But the result already carries these, so we could use ids.map(id => {... return {..., configured: ticks && axisSpec && axisTicksDimensions && axisPosition}}).filter(({configured}) => configured) - not sure if configured is the right name - or just make the map return null when a condition fails and then .filter(identity) where const identity = d => d

Ok that's fine, I just tend to use reduce to avoid multiple loops. What is the performance difference for large arrays using 1 loop vs 2+?

Ok so it looks like ts is not smart enough to realize the filter checks the fields from the map.get are defined.

I would need to return a Partial or do some typescript trickery

Are you ok keeping this as a reduce for this case?

Sure thing, and I think we don't really have guidelines for this level of coding questions anyway

Absolutely, esp. if the retained set can be large.

microsoft/TypeScript#33397 👍

Fixed in 1f27b60

monfera

Functionality looks good to me, does what the issue asks, code looks good, pending CI and PR checkbox ticks. As I'm a new reviewer in this code base pls. @markov00 also take a look.

src/chart_types/xy_chart/store/chart_state.ts

markov00

~~Let's also add a check on the number of desired ticks in the axis spec before merge this~~

markov00 · 2019-09-16T15:03:11Z

src/components/react_canvas/reactive_chart.tsx

+        ticks: axesVisibleTicks.get(id),
+        axisSpec: axesSpecs.get(id),
+        axisTicksDimensions: axesTicksDimensions.get(id),
+        axisPosition: axesPositions.get(id),


you can map this to something that respect the props of the <Axis> component like {key, ticks, axisSpec, axisTickDimensions, axisPosition }` so then you can just render it like

<Axis {...axis} chartTheme={chartTheme} debug={debug} chartDimensions={chartDimensions} />

see 2474776

markov00 · 2019-09-16T15:06:32Z

src/chart_types/xy_chart/store/chart_state.ts

+    ) {
+      const spec = specMap.get(axisId);
+
+      if (spec && spec.position === position && ((!title && !spec.title) || title === spec.title)) {


can we simplify this using spec && spec.position === position && title == spec.title?

see 2474776

markov00

left a comment to address

markov00 · 2019-09-18T09:17:12Z

src/chart_types/xy_chart/store/chart_state.ts

@@ -134,7 +134,7 @@ export const isDuplicateAxis = (
    ) {
      const spec = specMap.get(axisId);

-      if (spec && spec.position === position && ((!title && !spec.title) || title === spec.title)) {
+      if (spec && spec.position === position && title === spec.title) {


it's a double equal here if we want to mark as duplicate if one title is null and the other undefined (that in our case we can threat as no value)

Suggested change

if (spec && spec.position === position && title === spec.title) {

if (spec && spec.position === position && title == spec.title) {

The value cannot be null based on the types correct?

Nonetheless, I also don't like double equals in javascript because it is loose equality, and generally a bad practice. It also can cause unwanted conversions producing inaccurate results. I'd like to enforce this throughout the code eventually.

I think the explicit equality adding twice the code to clearly indicate the comparison is worth it.

yep that value cannot be null, but I was thinking on the case the chart used in a JS world (people can do strange things there).
You are right about the loose equality, I use that only in rare case checking when checking for null and undefined, in that case you are right, we can get some strange results. So please ignore my comment here

To your point about people using this library in javaScript and not minding the types.

I 100% agree, but I think if we want to address that there are numerous places where we assume values to be provided/required. I think the Theme is a perfect example where we assume options are provided on the type were in js they may not.

markov00 · 2019-09-18T09:18:34Z

src/components/react_canvas/reactive_chart.tsx

-        chartDimensions={chartDimensions}
-      />
+    return axes.map(({ key, ...axisProps }) => (
+      <Axis {...axisProps} key={key} chartTheme={chartTheme} debug={debug} chartDimensions={chartDimensions} />


Have you extracted the key a bit more explicit on the code?

Sorry what do you mean by this?

I tried spreading the props with the key included and I get a linting error not adding the key to mapd child. Is that what you mean?

yes, sorry :(
strange that the ts linter doesn't know about that

Ya seems like they can get the keys of the object to verify

markov00 · 2019-09-19T09:26:45Z

jenkins, test this please

# [12.1.0](v12.0.2...v12.1.0) (2019-09-19) ### Features * **axis:** option to hide duplicate axes ([#370](#370)) ([ada2ddc](ada2ddc)), closes [#368](#368)

markov00 · 2019-09-19T16:42:09Z

🎉 This PR is included in version 12.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [12.1.0](elastic/elastic-charts@v12.0.2...v12.1.0) (2019-09-19) ### Features * **axis:** option to hide duplicate axes ([opensearch-project#370](elastic/elastic-charts#370)) ([4381734](elastic/elastic-charts@4381734)), closes [opensearch-project#368](elastic/elastic-charts#368)

feat(axis): option to hide duplicate axes

37808ff

* Option to hide axes based on tick labels, position and title. * Refactor axes render function. closes elastic#368

nickofthyme requested review from markov00 and monfera September 10, 2019 22:33

nickofthyme added :axis Axis related issue enhancement New feature or request :TSVB Kibana TSVB related labels Sep 10, 2019

monfera reviewed Sep 11, 2019

View reviewed changes

nickofthyme added 3 commits September 12, 2019 08:56

Merge branch 'master' into feat/remove-duplicate-axes

e07b954

fix: pr comments

f306a93

test(chart_state): add tests for isDiplicateAxis function

add9661

monfera approved these changes Sep 12, 2019

View reviewed changes

src/chart_types/xy_chart/store/chart_state.ts Show resolved Hide resolved

nickofthyme added 2 commits September 12, 2019 12:10

refactor: update reduce to map.filter

37c4f0b

Merge branch 'master' into feat/remove-duplicate-axes

1f27b60

markov00 reviewed Sep 16, 2019

View reviewed changes

markov00 self-requested a review September 16, 2019 15:22

markov00 approved these changes Sep 16, 2019

View reviewed changes

fix: pr comments

2474776

markov00 reviewed Sep 18, 2019

View reviewed changes

nickofthyme merged commit ada2ddc into elastic:master Sep 19, 2019

nickofthyme deleted the feat/remove-duplicate-axes branch September 19, 2019 16:38

markov00 pushed a commit that referenced this pull request Sep 19, 2019

chore(release): 12.1.0 [skip ci]

53a0fe0

# [12.1.0](v12.0.2...v12.1.0) (2019-09-19) ### Features * **axis:** option to hide duplicate axes ([#370](#370)) ([ada2ddc](ada2ddc)), closes [#368](#368)

markov00 added the released Issue released publicly label Sep 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(axis): option to hide duplicate axes #370

feat(axis): option to hide duplicate axes #370

nickofthyme commented Sep 10, 2019 •

edited

Loading

codecov-io commented Sep 10, 2019 •

edited

Loading

monfera Sep 11, 2019

nickofthyme Sep 12, 2019 •

edited

Loading

monfera Sep 12, 2019

monfera Sep 11, 2019

nickofthyme Sep 12, 2019

monfera Sep 11, 2019

nickofthyme Sep 12, 2019

nickofthyme Sep 12, 2019

monfera Sep 12, 2019

monfera Sep 12, 2019

nickofthyme Sep 12, 2019

nickofthyme Sep 12, 2019

monfera left a comment

markov00 left a comment •

edited

Loading

markov00 Sep 16, 2019

nickofthyme Sep 16, 2019

markov00 Sep 16, 2019

nickofthyme Sep 16, 2019

markov00 left a comment

markov00 Sep 18, 2019

nickofthyme Sep 18, 2019

markov00 Sep 18, 2019

nickofthyme Sep 19, 2019

markov00 Sep 18, 2019

nickofthyme Sep 18, 2019

markov00 Sep 18, 2019

nickofthyme Sep 18, 2019

markov00 commented Sep 19, 2019

markov00 commented Sep 19, 2019

	if (spec && spec.position === position && title === spec.title) {
	if (spec && spec.position === position && title == spec.title) {

feat(axis): option to hide duplicate axes #370

feat(axis): option to hide duplicate axes #370

Conversation

nickofthyme commented Sep 10, 2019 • edited Loading

Summary

Checklist

codecov-io commented Sep 10, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

nickofthyme Sep 12, 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

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

monfera left a comment

Choose a reason for hiding this comment

markov00 left a comment • 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

markov00 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 commented Sep 19, 2019

markov00 commented Sep 19, 2019

nickofthyme commented Sep 10, 2019 •

edited

Loading

codecov-io commented Sep 10, 2019 •

edited

Loading

nickofthyme Sep 12, 2019 •

edited

Loading

markov00 left a comment •

edited

Loading