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(tokens): warn about old react-tokens usage #739

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

adamviktora
Copy link
Contributor

Closes #733

Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

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

Looks great! One non-blocking thought:


const { imports: tokenSpecifiers } = getFromPackage(context, tokensPackage);

const defaultTokensWithDeclaration = getDefaultImportsFromPackage(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think this might be something worth helperizing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if we ever use it again, I feel like it is quite specific. Do you have some potential future use case on your mind?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To clarify I don't mean for tokens explicitly, but something that returns the default imports file name and the import declaration in a nice easy to use format like this.

I feel like I've had a few places where it would've been nice so far, though I haven't dug through to find what those instances were. If you don't think it would likely be helpful it's always something we can revisit in the future if it turns out the other way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you are right, I also ran to something similar maybe on the component-groups. I lean towards creating the helper as a followup, if we run to a problem where it would be helpful

Copy link
Collaborator

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Quick comment below, otherwise lgtm

Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>
@adamviktora
Copy link
Contributor Author

adamviktora commented Aug 22, 2024

Also, we should maybe not only check react-tokens, but also raw strings having the token values in the codebase.

We could also check CSS files, but that probably is not achievable by ESLint.

@thatblindgeye thatblindgeye merged commit 1e685a2 into patternfly:main Aug 22, 2024
3 checks passed
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.

Provide warning for consumers that are using our old CSS tokens
3 participants