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

chore: remove enums from project #266

Merged
merged 5 commits into from
Jul 17, 2019
Merged

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Jul 16, 2019

Summary

This PR removes all the remaining enums on the project as babel doesn't support const enum directly.
The refactoring doesn't create any breaking change, just a bit more verbose code.

@emmacunningham I've also seen a problem with the AnnotationType , not resolved in this PR but I can work on it:

export const AnnotationTypes = Object.freeze({
  Line: 'line' as AnnotationType,
  Rectangle: 'rectangle' as AnnotationType,
  Text: 'text' as AnnotationType,
});

export type AnnotationType = 'line' | 'rectangle' | 'text';

in this way, if you specify something like

const annotation = AnnotationTypes.Line

the type of annotation is AnnotationType and not line that means that if you want to do typechecking on that variable needs to go in a different way. I've fixed that behaviour specifying the exact type for every object value like Bottom: 'bottom' as 'bottom' and I've reused the typeof Position.Bottom instead of rewriting the string type. Let me know what do you think

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

@markov00 markov00 added the chore label Jul 16, 2019
@markov00 markov00 requested review from nickofthyme and emmacunningham and removed request for nickofthyme July 16, 2019 17:55
@codecov-io
Copy link

codecov-io commented Jul 16, 2019

Codecov Report

Merging #266 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
- Coverage   98.12%   98.11%   -0.01%     
==========================================
  Files          36       37       +1     
  Lines        2662     2654       -8     
  Branches      607      621      +14     
==========================================
- Hits         2612     2604       -8     
  Misses         45       45              
  Partials        5        5
Impacted Files Coverage Δ
src/lib/series/curves.ts 100% <100%> (ø) ⬆️
src/lib/utils/interactions.ts 100% <100%> (ø) ⬆️
src/lib/themes/theme.ts 100% <100%> (ø) ⬆️
src/state/chart_state.ts 97.45% <100%> (ø) ⬆️
src/lib/series/specs.ts 100% <100%> (ø) ⬆️
src/lib/utils/scales/scales.ts 100% <100%> (ø)
src/state/annotation_utils.ts 100% <0%> (ø) ⬆️
src/lib/series/domains/y_domain.ts 100% <0%> (ø) ⬆️
src/lib/series/domains/x_domain.ts 100% <0%> (ø) ⬆️
src/lib/series/scales.ts 100% <0%> (ø) ⬆️
... and 6 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 a8f99b1...c3bfa74. Read the comment docs.

Copy link
Contributor

@emmacunningham emmacunningham left a comment

Choose a reason for hiding this comment

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

code lgtm. the approach of explicitly assigning each value to itself and then using typeof in the union type definition looks good and can be extended for the annotation types.

@markov00 markov00 changed the title Remove enums from project chore: remove enums from project Jul 17, 2019
@markov00 markov00 merged commit ebe9d3f into elastic:master Jul 17, 2019
@markov00 markov00 deleted the remove_enums branch July 17, 2019 15:57
@markov00
Copy link
Member Author

🎉 This PR is included in version 8.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants