-
Notifications
You must be signed in to change notification settings - Fork 371
chore(chart bar): convert to typescript #11727
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(chart bar): convert to typescript #11727
Conversation
Preview: https://patternfly-react-pr-11727.surge.sh A11y report: https://patternfly-react-pr-11727-a11y.surge.sh |
e2a9c35
to
a39876d
Compare
/* eslint-disable-next-line */ | ||
import t_global_color_status_danger_100 from '@patternfly/react-tokens/dist/esm/t_global_color_status_danger_100'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure you had brought this issue up with me on Slack so if we've already discussed this just let me know. But I'm wondering if we should just update the eslint config to try and ignore these token names. @kmcfaul wdyt? Testing out locally if we were to update the camelcase rule to add:
allow: ["^t_[global|chart]"]
it should ignore these sorts of imports without having to add all the ignore comments. We'd just have to update imports of charts_
tokens to the t_charts_
ones, but curious if there's potential for things to sneak in that you can think of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we had a discussion regarding this in slack, I previously used eslint-disable camelcase
. You suggested to use eslint-disable-next-line
instead.
This issue might occur again with other chart components in the future PRs.
I have also seen the usage of eslint-disable camelcase
in various existing examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mash707 for now let's try making the above update in the eslint config. Having a bunch of ignore comments in these files will add up and create noise we don't really need.
We could also allow usage of charts_
imports/tokens, since updating those is a bit out of scope here (would be worth a "TODO" comment mentioning we should update the allows array to remove that one after updating usage in charts examples)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, will update the eslint config.
a39876d
to
4f2ba8d
Compare
Towards #11719
The following examples of ChartBar will be converted to TypeScript: