Skip to content

Conversation

Mash707
Copy link
Contributor

@Mash707 Mash707 commented Mar 29, 2025

Towards #11719

The following examples of ChartBar will be converted to TypeScript:

  • Basic with right aligned legend
  • Purple with bottom aligned legend
  • Multi-color (ordered) with bottom-left aligned legend
  • Single with right aligned legend
  • Alerts timeline

@patternfly-build
Copy link
Contributor

patternfly-build commented Mar 29, 2025

@Mash707 Mash707 mentioned this pull request Mar 29, 2025
18 tasks
@Mash707 Mash707 force-pushed the convert-chart-bar-to-typescript branch from e2a9c35 to a39876d Compare April 9, 2025 10:32
/* eslint-disable-next-line */
import t_global_color_status_danger_100 from '@patternfly/react-tokens/dist/esm/t_global_color_status_danger_100';
Copy link
Contributor

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.

Copy link
Contributor Author

@Mash707 Mash707 Apr 9, 2025

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.

Copy link
Contributor

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)

Copy link
Contributor Author

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.

@Mash707 Mash707 force-pushed the convert-chart-bar-to-typescript branch from a39876d to 4f2ba8d Compare April 23, 2025 20:31
@Mash707 Mash707 requested a review from thatblindgeye April 28, 2025 17:33
@thatblindgeye thatblindgeye merged commit 31a4c28 into patternfly:main Apr 28, 2025
13 checks passed
@Mash707 Mash707 deleted the convert-chart-bar-to-typescript branch April 29, 2025 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants