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

refactor: scale improvements #1383

Merged
merged 79 commits into from
Sep 22, 2021
Merged

refactor: scale improvements #1383

merged 79 commits into from
Sep 22, 2021

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Sep 17, 2021

Summary

Switch to TS 4.4; simplifications; type strength increase via lots of any replacements. Replacement and future prevention of coercive == and != ops

BREAKING CHANGE

The public type variations for domains are discontinued, in favor of retaining the single DomainRange export, which however now has a mandatory {min: number, max: number}. The user can supply NaN where a finite min, max or both aren't defined (ie. in place of former omisison or undefined).
Some console.warn punctuations changed.

Details

  • All anys removed from continuous and band scales; one any removed from Scale. Some of these any fixes needed some new type assertions in more specific, localized places (very low count), future PRs can attempt to solve those too with type guards or generics.
  • Lots of code refactoring, optionality removal, parameter removal, function removal, and removal of some ad-hoc types
  • 350 lines removed 👯‍♀️ if we don't count the yarn lock growth due to deps bump

Note to reviewers:

  • please double check the unit test changes if they're in line with the original intent of those cases
  • please check the type changes, esp. in place of anys, and the downstream type assertions necessitated by any removal, because the numerous preexisting (and still extant) anys mean that information can't always be learned from other types
  • ideas about, or follow-up PRs for the reduction of remaining any (eg. in Scale) and the as assertions, necessitated by the removal of the any instances are welcome, due to your experience with the Cartesian implementation and use cases within Elastic

Issues

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
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)

@monfera monfera added :axis Axis related issue :data Data/series/scales related issue :heatmap Heatmap/Swimlane chart related issue :xy Bar/Line/Area chart related code quality wip work in progress labels Sep 17, 2021
@monfera monfera force-pushed the time-axis-06 branch 3 times, most recently from d3d94a0 to 2d2142a Compare September 17, 2021 08:33
@monfera
Copy link
Contributor Author

monfera commented Sep 17, 2021

test this

1 similar comment
@monfera
Copy link
Contributor Author

monfera commented Sep 17, 2021

test this

@monfera
Copy link
Contributor Author

monfera commented Sep 17, 2021

test this

@monfera
Copy link
Contributor Author

monfera commented Sep 17, 2021

test this

@monfera
Copy link
Contributor Author

monfera commented Sep 17, 2021

test this

@monfera monfera removed the wip work in progress label Sep 17, 2021
@monfera monfera requested a review from nickofthyme September 17, 2021 21:38
@monfera
Copy link
Contributor Author

monfera commented Sep 21, 2021

test this

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Latest code changes LGTM! Great work Robert!!

export type UnboundedDomainWithInterval = DomainBase;

/** @public */
export type DomainRange = LowerBoundedDomain | UpperBoundedDomain | CompleteBoundedDomain | UnboundedDomainWithInterval;
/** @public */
export type YDomainRange = YDomainBase & DomainRange & LogScaleOptions;

Copy link
Collaborator

Choose a reason for hiding this comment

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

I failed to find an simple solution to this in the form of some deep partial keys on the spec factory. I'll keep the PR here for reference. #1399

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.

Great work here Robert, thanks for taking care of this.
I failed miserably the last time I've tried typing the scale, but you did it an awesome job here!

@monfera monfera merged this pull request into master Sep 22, 2021
@monfera monfera deleted the time-axis-06 branch September 22, 2021 09:44
monfera added a commit that referenced this pull request Sep 22, 2021
* switch to TS 4.4 and bump other deps
* type strength increase via removing many `any` occurrences
* domain oriented generic typing for scales
* replacement and future prevention of coercive `==` and `!=` ops
* refactoring: simplification and shortening of numerous functions, about 300 runtime lines gone
* removal of numerous let, if/then, optional parameters etc.

BREAKING CHANGE: The public type varieties for domains are discontinued, in favor of retaining the single `DomainRange` export, which now has a mandatory `{min: number, max: number}`. The developer can supply `NaN` where a finite min, max or both aren't defined (ie. in place of former effective `undefined`). In addition, some console.warn punctuations changed

Co-authored-by: Marco Vettorello <[email protected]>
Co-authored-by: Nick Partridge <[email protected]>
nickofthyme added a commit that referenced this pull request Oct 5, 2021
# [37.0.0](v36.0.0...v37.0.0) (2021-10-05)

### Bug Fixes

* **debug:** add predictable axis labels sorting order ([#1418](#1418)) ([60fbe7a](60fbe7a))
* **deps:** update dependency @elastic/eui to ^38.1.0 ([#1409](#1409)) ([4ffd018](4ffd018))
* **deps:** update dependency @elastic/eui to ^38.2.0 ([#1416](#1416)) ([34707c3](34707c3))

### Code Refactoring

* cleanup colors ([#1397](#1397)) ([348c061](348c061))
* scale improvements and TS 4.4 ([#1383](#1383)) ([0003bc1](0003bc1))

### BREAKING CHANGES

* `DEFAULT_CHART_MARGINS`, `DEFAULT_GEOMETRY_STYLES`, `DEFAULT_CHART_PADDING` and `DEFAULT_MISSING_COLOR` are no longer exposed as part of the API
* The public type varieties for domains are discontinued, in favor of retaining the single `DomainRange` export, which now has a mandatory `{min: number, max: number}`. The developer can supply `NaN` where a finite min, max or both aren't defined (ie. in place of former effective `undefined`). In addition, some console.warn punctuations changed

Co-authored-by: Marco Vettorello <[email protected]>
Co-authored-by: Nick Partridge <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:axis Axis related issue code quality :data Data/series/scales related issue :heatmap Heatmap/Swimlane chart related issue :xy Bar/Line/Area chart related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants