Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(scales): add LinearBinary scale type #1389

Merged
merged 6 commits into from
Oct 6, 2021

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Sep 20, 2021

Summary

This PR adds LienarBinary to ScaleTypes. This scale type functions exactly like Linear with the exception of ticks which are computed in a base 2 rather than base 10.

image

Details

Overrides ticks and nice methods for linear scales in packages/charts/src/scales/scale_continuous.ts. This is a temporary work around until changes are added to d3-array and d3-scale.

See example story here, once deployed.

Issues

This completes a missing feature requested by the Lens team regarding binary ticks
closes #1395

Related to elastic/kibana#97100, elastic/kibana#7539

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated

@nickofthyme nickofthyme added wip work in progress ci:skip Skip all build checks labels Sep 20, 2021
@nickofthyme nickofthyme changed the title feat(axis): add options for linear binary ticks feat(axis): add binary scale type Sep 20, 2021
@nickofthyme nickofthyme changed the title feat(axis): add binary scale type feat(scales): add LinearBinary scale type Sep 27, 2021
@nickofthyme nickofthyme added :axis Axis related issue :data Data/series/scales related issue :xy Bar/Line/Area chart related enhancement New feature or request and removed ci:skip Skip all build checks wip work in progress labels Sep 27, 2021
@nickofthyme nickofthyme marked this pull request as ready for review September 27, 2021 17:46
@nickofthyme nickofthyme requested a review from monfera September 27, 2021 18:41
Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

LGTM!

Would still like to farm out our copy of D3 derived code so our license is consistent in charts/ or at least charts/src @markov00 seeking your input to this too

I see this PR exercises an ability for Charts to ingest strings for numbers, it's not this PR's making though

Minor code comments inside

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

LGTM I tested it locally, reducing the size of the ticks and applying a brutal byte formatter and it works correctly.
I'm not sure what cause the VRT failure visible on the CI

@nickofthyme nickofthyme enabled auto-merge (squash) October 6, 2021 16:48
@nickofthyme nickofthyme merged commit 9f2e427 into elastic:master Oct 6, 2021
nickofthyme pushed a commit that referenced this pull request Oct 15, 2021
# [38.0.0](v37.0.0...v38.0.0) (2021-10-15)

### Bug Fixes

* **deps:** update dependency @elastic/eui to v39 ([#1422](#1422)) ([2ee97aa](2ee97aa))
* **goal:** reduce whitespace for circular charts ([#1413](#1413)) ([6517523](6517523))
* **interactions:** change allowBrushingLastHistogramBin to true ([#1396](#1396)) ([9fa9783](9fa9783))
* **xy:** remove wrongly represented null/missing values in tooltip ([#1415](#1415)) ([e5963a3](e5963a3)), closes [#1414](#1414)

### Code Refactoring

* scales ([#1410](#1410)) ([a53a2ba](a53a2ba))

### Features

* **scales:** add `LinearBinary` scale type ([#1389](#1389)) ([9f2e427](9f2e427))
* **xy:** adaptive tick raster ([#1420](#1420)) ([200577b](200577b))
* **xy:** apply the data value formatter to data values over bars ([#1419](#1419)) ([e673fc7](e673fc7))

### BREAKING CHANGES

* **interactions:** allowBrushingLastHistogramBucket renamed to allowBrushingLastHistogramBin on the Settings component defaults true and is only applied for histogram type charts
* LogScaleOptions.logBase` is now a `number` instead of the object enum `LogBase`. Some edge case data or configuration _might_, with a deemed low likelihood, lead to a situation where the earlier version would have silently not rendered a bar, line or point, while the new code doesn't `catch`, therefore throw an exception (see the last item). General risk of regressions due to the quantity of code changes (altogether 3.5k)
@nickofthyme
Copy link
Collaborator Author

🎉 This PR is included in version 38.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Oct 15, 2021
@nickofthyme nickofthyme deleted the mock-binary-scale branch March 7, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:axis Axis related issue :data Data/series/scales related issue enhancement New feature or request released Issue released publicly :xy Bar/Line/Area chart related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better tick placement for scales with binary tick formatting
3 participants