patternfly / patternfly-react

A set of React components for the PatternFly project.
https://react-staging.patternfly.org/
MIT License
777 stars 353 forks source link

Revamp react-tokens logic #10212

Open wise-king-sullyman opened 6 months ago

wise-king-sullyman commented 6 months ago

Describe the enhancement or change Our token generation in the react-tokens package needs to be reworked as the logic we use to determine css var values doesn't match 1:1 with how that value will be determined in browser, this can lead to build breaking bugs with recursively defined values.

We've implemented a temporary fix for this, but it is still not optimal as differences in behavior could occur if a var is defined multiple times in a stylesheet. Ideally we should only parse the css within the root selector of the stylesheet.

Additionally, core currently has to add some variables to the root scope for the sole purpose of having the token created in react-tokens, ideally this will not be required in the future.

mcoker commented 6 months ago

We've implemented a temporary fix for this, but it is still not optimal as differences in behavior could occur if a var is defined multiple times in a stylesheet. Ideally we should only parse the css within the root selector of the stylesheet.

Assuming the purpose of the token is to retrieve the style for a given variable, all of the component variables we want to expose to users will be within the top/root block of variables for a component, and there should be no duplications. Any use/reference of variables outside of that block shouldn't be reflected in react-tokens, so this is a good change.

Additionally, core currently has to add some variables to the root scope for the sole purpose of having the token created in react-tokens, ideally this will not be required in the future.

To elaborate a little, we may use an undefined CSS variable outside of the root block as part of some component style, because some other component style will provide the value for that variable. Since the react tokens script currently parses a component's entire stylesheet, and fails on any used - but undefined - vars, we have to define these vars in a way that makes react-tokens happy, but doesn't provide any style benefit. For example, in the block below, --pf-c-button--background does not need to be defined with a default value, because a button will always be used with a modifier (eg, pf-m-primary), and the modifier will define --pf-c-button--background. But if we don't define it, when react tokens sees background: var(--pf-c-button--background);, it fails because -pf-c-button--background is not defined.

:root {
  --pf-c-button--background: transparent; // this only exists to make react-tokens happy
  --pf-c-button--m-primary--background: blue;
}

.pf-c-button {
  background: var(--pf-c-button--background);

 &.pf-m-primary {
    --pf-c-button--background: var(--pf-c-button--m-primary--background);
 }
}
tlabaj commented 6 months ago

Lets use this issue for a spike and spend a day figuring put what we want to do. WE can open a follow development issue once the spike is done.

github-actions[bot] commented 4 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] commented 2 months ago

This issue has been automatically marked as stale because it has not had recent activity.

github-actions[bot] commented 1 day ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.