feat: nice ticks everywhere by markov00 · Pull Request #1087 · elastic/elastic-charts

markov00 · 2021-03-23T15:57:42Z

Summary

This PR adds the nicing to the scales domain.
As the first step, I've just introduced an optional nice prop per axis, off by default to avoid breaking change now.

export interface SeriesScales {
    ...
    xScaleType: XScaleType;
    xNice?: boolean;
    yScaleType: ScaleContinuousType;
    yNice?: boolean;
}

The following improvements and refactoring are implemented:

the scale default parameters are now stored into scale_defaults.ts.
we have now a selector that merges and computes the final "configuration layer" where we collect, merge, cast all the scales related configuration from the current API (merging configurations from the SettingsSpec, AxisSpec, SeriesSpec and the all relative default values).

This is the first step toward refactoring the scale-related configuration into its own component.

fix #397

codecov-io · 2021-04-01T13:13:30Z

Codecov Report

Merging #1087 (5aa72fc) into master (282082b) will increase coverage by 0.14%.
The diff coverage is 94.52%.

❗ Current head 5aa72fc differs from pull request most recent head 6ec6b3e. Consider uploading reports for the commit 6ec6b3e to get more accurate results

@@            Coverage Diff             @@
##           master    #1087      +/-   ##
==========================================
+ Coverage   71.55%   71.70%   +0.14%     
==========================================
  Files         381      402      +21     
  Lines       11914    11527     -387     
  Branches     2588     2205     -383     
==========================================
- Hits         8525     8265     -260     
+ Misses       3358     3131     -227     
- Partials       31      131     +100

Flag	Coverage Δ
unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../chart_types/heatmap/layout/viewmodel/viewmodel.ts	`4.57% <0.00%> (+0.20%)`	⬆️
...atmap/state/selectors/get_x_axis_right_overflow.ts	`38.09% <0.00%> (ø)`
...y_chart/state/selectors/compute_axes_geometries.ts	`100.00% <ø> (ø)`
...xy_chart/state/selectors/merge_y_custom_domains.ts	`100.00% <ø> (ø)`
src/chart_types/xy_chart/utils/axis_utils.ts	`93.46% <ø> (-1.46%)`	⬇️
src/chart_types/xy_chart/utils/specs.ts	`100.00% <ø> (ø)`
...types/heatmap/state/selectors/get_heatmap_table.ts	`34.48% <50.00%> (+4.48%)`	⬆️
src/chart_types/xy_chart/scales/get_api_scales.ts	`87.50% <87.50%> (ø)`
src/mocks/xy/domains.ts	`89.47% <89.47%> (ø)`
src/chart_types/xy_chart/domains/x_domain.ts	`98.91% <97.14%> (-1.09%)`	⬇️
... and 238 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 282082b...6ec6b3e. Read the comment docs.

src/scales/scale_continuous.ts

nickofthyme

Code changes LGTM. Tested locally on storybook and functions as expected.

Can we add a nice example for time scales?

src/chart_types/heatmap/state/selectors/get_heatmap_table.ts

src/chart_types/xy_chart/domains/y_domain.test.ts

src/chart_types/xy_chart/state/selectors/compute_axes_geometries.ts

stories/axes/8_custom_domain.tsx

src/chart_types/xy_chart/domains/x_domain.ts

…0/elastic-charts into 2021_03_23-nice_ticks_everywhere

monfera · 2021-04-07T20:30:29Z

api/charts.api.md

 export type AnnotationType = $Values<typeof AnnotationType>;

+// @public (undocumented)
+export interface APIScale<T extends ScaleType> {


Not sure of the pros/cons of putting the word API in an element of the API, all things being equal, would prefer a simple, expressive, semantic name. I'd probably not call it Scale because it's currently just a tuple of {type: 'linear' | ..., nice: boolean}

Right, any suggestion? what if we don't expose the interface externally on our API, and we reserve the APIScaletype for internal purposes?
The external will look like this:

export interface SeriesScales { ... xScaleType: XScaleType | { nice: boolean; type: XScaleType }; yScaleType: ScaleContinuousType | { nice: boolean; type: ScaleContinuousType }; }

I think, even for internal use the word "API" in it is a good question, maybe it has a bunch of benefits, eg. knowing which "boundary layer" we talk about, eg. external API, internal API or implementation detail (the latter of which is clear from @internal).

Could namespaces / modules achieve this without resorting to Hungarian notation?

Is this inception or recursion?

monfera

Epic PR 🎉

Some of my comments might be discoursive, and for consideration of future refactoring and API changes. Those that directly relate to new structures esp. in the API, eg. trying to avoid a union type in xScaleType or some public namings pertain to this PR more closely. These could be discussed if necessary, as ideally we don't add to the API to then soon revise it

monfera · 2021-04-07T20:44:21Z

api/charts.api.md

@@ -1669,10 +1677,10 @@ export type SeriesNameFn = (series: XYChartSeriesIdentifier, isTooltip: boolean)
 // @public (undocumented)
 export interface SeriesScales {


Unrelated to the PR:

instead of SeriesScales, we might eventually consider a more specific term, eg. ProjectionPair or CartesianPlaneProjections (or a shorter version), to express that it's not some set or array of scales, but it defines the Cartesian plane, with the orthogonal projection pair

also, "projection" might work better here, and also in a lot of places where we use scale at the moment; Projection captures the linearity (or how it's non-linear) of a dimension, eg. current ScaleType; while Scale adds the multiplier and usually offset, for example, by using the data domain and the screen pixel range

monfera · 2021-04-07T20:50:49Z

api/charts.api.md

 export type AnnotationType = $Values<typeof AnnotationType>;

+// @public (undocumented)
+export interface APIScale<T extends ScaleType> {


I think, even for internal use the word "API" in it is a good question, maybe it has a bunch of benefits, eg. knowing which "boundary layer" we talk about, eg. external API, internal API or implementation detail (the latter of which is clear from @internal).

Could namespaces / modules achieve this without resorting to Hungarian notation?

api/charts.api.md

monfera · 2021-04-07T21:26:26Z

src/chart_types/heatmap/layout/viewmodel/viewmodel.ts


  const timeScale =
-    xDomain.scaleType === ScaleType.Time
+    xDomain.type === ScaleType.Time


The word domain covers, or maybe in our case, characterizes the set of values that a variable can assume, and is only very loosely connected to the projection type which is what we mean here, eg. linear, logarithmic. Not just in math, but also in geography and eg. D3, domain and projection are quite independent.

This has practical use too. For example, it'll be eventually useful to describe/configure the domain as a first class entity (and codomain, which we can call yDomain), and share it among multiple visualizations, some of which may use linear projection, some, logarithmic projection, and some, square root projection (eg. for varying size heatmap squares).

Domains compose nicely, eg. the discussed operators, like

unioning (not just enlarging the domain if the user wants, but also, projecting multiple series on the same cartesian dimesion, we need to compute the union of the domains of all the series)

mandatory inclusion of a certain obligatevalue, eg. zero—which is of course just a special case of unioning (unioning with a set that has a singluar value)

intersection of the domain, eg. crop outliers (maybe shown or listed separately or on an overview chart) etc

we can clean this up in a different PR.
We should change not only the variable name, but also the enum

Yes that's good, do you have a list of items on which this can be put, to eventually circle back if deemed useful?

monfera · 2021-04-07T21:28:56Z

src/chart_types/heatmap/layout/viewmodel/viewmodel.ts

            type: ScaleType.Time,
            domain: xDomain.domain,
            range: [0, chartDimensions.width],
+            nice: false,


Yes, the nice flag forms high cohesion with the domain of the data and the pixel range onto which we project, as these are equally needed to compute the ticks. Also, the proposed tick count, or tick density (eg. via max font height and padding, avoiding overlap or too many/too few ticks)

monfera · 2021-04-07T21:56:59Z

src/chart_types/xy_chart/state/selectors/get_api_scale_configs.ts

+}
+
+/** @internal */
+export const getAPIScaleConfigsSelector = createCachedSelector(


Maybe both the get prefix and the Selector postfix could be removed as they're Hungarian notation

monfera · 2021-04-07T22:00:45Z

src/chart_types/xy_chart/state/selectors/get_api_scale_configs.ts

+    return acc;
+  }, {});
+
+  const customDomainByGroupId = mergeYCustomDomainsByGroupId(axisSpecs, settingsSpec.rotation);


merge in mergeYCustomDomainsByGroupId is a verb, while functions stand in place of the equivalent expression or value, so a noun eg. unionYCustomDomainsByGroupId would be a better fit in this otherwise very nice, descriptive function name (OK union can be either a word or a noun, but at least, it is possibly a noun)

src/chart_types/xy_chart/state/selectors/get_api_scale_configs.ts

src/chart_types/xy_chart/utils/specs.ts

…0/elastic-charts into 2021_03_23-nice_ticks_everywhere

monfera

Thanks for the changes, it looks great 🎉 Tested with negative numbers, maybe useful to add some negatives to the Custom Domains story if you need to touch this story in a future PR, and separate nicing toggle per series (ie. everything bar would be on the bar knob page etc)

nickofthyme

Changes per @monfera comments look good to me.

nickofthyme · 2021-04-12T17:36:58Z

api/charts.api.md

# [28.1.0](v28.0.1...v28.1.0) (2021-04-13) ### Bug Fixes * **legend:** sizing for short labels with scrollbar ([#1115](#1115)) ([6e1f223](6e1f223)) * **xy:** negative bar highlight and click ([#1109](#1109)) ([ec17cb2](ec17cb2)), closes [#1100](#1100) ### Features * **a11y:** improve chart figure ([#1104](#1104)) ([815cf39](815cf39)) * **partition:** order slices and sectors ([#1112](#1112)) ([74df29b](74df29b)) * **partitions:** small multipies events pass on smAccessorValue ([#1106](#1106)) ([a3234fe](a3234fe)) * **xy:** optionally rounds the domain to nice values ([#1087](#1087)) ([f644cc4](f644cc4)) * **xy:** specify pixel and ratio width for bars ([#1114](#1114)) ([58de413](58de413)) * mosaic ([#1113](#1113)) ([64bdd88](64bdd88))

nickofthyme · 2021-04-13T14:28:19Z

🎉 This PR is included in version 28.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

the elastic#1087 PR introduced a regression where the useDefaultGroupDomain was not considered when computing the scale configs.

The #1087 PR introduced a regression where the useDefaultGroupDomain was not considered when computing the scale configs.

# [28.2.0](v28.1.0...v28.2.0) (2021-04-15) ### Bug Fixes * **xy:** consider `useDefaultGroupDomain` on scale config ([#1119](#1119)) ([c1b59f2](c1b59f2)), closes [#1087](#1087) ### Features * **a11y:** allow user to pass custom description for screen readers ([#1111](#1111)) ([2ee1b91](2ee1b91)), closes [#1097](#1097) * **partition:** add debuggable state ([#1117](#1117)) ([d7fc206](d7fc206)), closes [#917](#917)

feat: nice ticks everywhere

fe47338

markov00 added wip work in progress :axis Axis related issue labels Mar 23, 2021

markov00 added 3 commits March 29, 2021 11:06

feat: add nice prop on scale type

acee82b

refactor: compute scale API config separately

521a15f

test: fix wrong test change

5aa72fc

markov00 added 4 commits April 6, 2021 15:04

fix: reduce global scale correctly

6ec6b3e

fix: stories

5d2f1a8

Merge branch 'master' into 2021_03_23-nice_ticks_everywhere

c063d22

chore: fix lint and api docs

08527a0

markov00 commented Apr 7, 2021

View reviewed changes

src/scales/scale_continuous.ts Outdated Show resolved Hide resolved

markov00 added enhancement New feature or request and removed wip work in progress labels Apr 7, 2021

markov00 requested review from monfera, nickofthyme and rshen91 April 7, 2021 12:44

markov00 marked this pull request as ready for review April 7, 2021 12:45

Merge branch 'master' into 2021_03_23-nice_ticks_everywhere

5120ef8

nickofthyme approved these changes Apr 7, 2021

View reviewed changes

markov00 added 2 commits April 7, 2021 22:11

fix: review comments

b9db40c

Merge branch '2021_03_23-nice_ticks_everywhere' of github.com:markov0…

38ca96f

…0/elastic-charts into 2021_03_23-nice_ticks_everywhere

monfera reviewed Apr 7, 2021

View reviewed changes

Merge branch 'master' into 2021_03_23-nice_ticks_everywhere

e0416b5

monfera reviewed Apr 7, 2021

View reviewed changes

markov00 added 3 commits April 8, 2021 14:16

refactor: rename internal ticks in desiredTickCount

cedf1bb

refactor: split type and nice into separate props

c0060d0

Merge branch '2021_03_23-nice_ticks_everywhere' of github.com:markov0…

f0a9090

…0/elastic-charts into 2021_03_23-nice_ticks_everywhere

monfera self-requested a review April 12, 2021 11:49

monfera approved these changes Apr 12, 2021

View reviewed changes

markov00 added 2 commits April 12, 2021 18:28

Merge branch 'master' into 2021_03_23-nice_ticks_everywhere

7062f47

test: update VRT with negative values

2d16951

nickofthyme approved these changes Apr 12, 2021

View reviewed changes

api/charts.api.md

Comment on lines 1680 to 1686

Copy link
Copy Markdown

Collaborator

nickofthyme Apr 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

Merge branch 'master' into 2021_03_23-nice_ticks_everywhere

df26747

markov00 merged commit f644cc4 into elastic:master Apr 13, 2021

markov00 deleted the 2021_03_23-nice_ticks_everywhere branch April 13, 2021 08:26

nickofthyme added the released Issue released publicly label Apr 13, 2021

markov00 added a commit to markov00/elastic-charts that referenced this pull request Apr 14, 2021

fix(xy): consider useDefaultGroupDomain on scale config

0ff2d2f

the elastic#1087 PR introduced a regression where the useDefaultGroupDomain was not considered when computing the scale configs.

markov00 mentioned this pull request Apr 14, 2021

fix(xy): consider useDefaultGroupDomain on scale config #1119

Merged

2 tasks

markov00 added a commit that referenced this pull request Apr 15, 2021

fix(xy): consider useDefaultGroupDomain on scale config (#1119)

c1b59f2

The #1087 PR introduced a regression where the useDefaultGroupDomain was not considered when computing the scale configs.

markov00 mentioned this pull request Apr 15, 2021

[APM] Charts Y-axis doesn't show maxValue in the ticks elastic/kibana#84482

Closed

nickofthyme mentioned this pull request Apr 15, 2021

Update dependency @elastic/charts to v28.2.0 (master) elastic/kibana#97005

Merged

1 task

ryankeairns mentioned this pull request Feb 2, 2022

[Dashboard] [META] Aesthetic by default elastic/kibana#118429

Open

24 tasks

		@@ -1669,10 +1677,10 @@ export type SeriesNameFn = (series: XYChartSeriesIdentifier, isTooltip: boolean)
		// @public (undocumented)
		export interface SeriesScales {

Conversation

markov00 commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

codecov-io commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

monfera left a comment

Choose a reason for hiding this comment

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickofthyme commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

markov00 commented Mar 23, 2021 •

edited

Loading

codecov-io commented Apr 1, 2021 •

edited

Loading