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

Abstraction of style values into private css variables #2364

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

r100-stack
Copy link
Member

@r100-stack r100-stack commented Dec 6, 2024

Changes

This PR abstracts the style values out of the CSS declaration blocks and stores those style values as private CSS variables (similar to iui-field). This abstraction in a few components makes the code easier to expand in the future (e.g. #2373).

The benefit of this PR is less complicated selectors in #2373. For example, iTwinUI-css code sometimes has styles for different types of disabled ([disabled], :disabled, [aria-disabled="true"], [data-iui-disabled="true"]). Additionally, there are a quite a few cases to handle and multiple combinations between them. For instance, in tabs, a tab can be selected or not and could be green or not, which leads to four permutations.

Thus, with this PR, #2373 just needs to override the values of these private variables without having to re-write the complex cases already handled by iTwinUI-css code.


One of the reasons I thought to not directly override the tokens was because of nested components. Suppose we have an iTwinUI component X and it can take anything as a child. We also other iTwinUI components Y and Z. Then suppose we do a global mapping of --iui-color-text: --kiwi-color-text-neutral-primary and then locally override the token in component X by doing --iui-color-text: --kiwi-color-text-neutral-secondary since that has a unique text color. If component Y is now placed inside X, it may incorrectly use --kiwi-color-text-neutral-secondary instead of --kiwi-color-text-neutral-primary as its text color. We may not uncover this issue since our demos may never have component Y inside of component X. (sandbox to test this)

More explanations of why I added each set of variables: #2364 (comment).

Since the new CSS variables added are private, I didn't think adding more variables to avoid problems like this would be an issue.


Apart from the rearrangement of the code, no changes were made to the actual styles.

Testing

Ensured that no image tests are failing.

Docs

N/A since no user facing change.

After PR TODOs

@r100-stack r100-stack self-assigned this Dec 6, 2024
@mayank99 mayank99 mentioned this pull request Dec 17, 2024
5 tasks
@r100-stack r100-stack marked this pull request as ready for review December 17, 2024 18:38
@r100-stack r100-stack requested a review from a team as a code owner December 17, 2024 18:38
@r100-stack r100-stack requested review from mayank99 and smmr-dn and removed request for a team December 17, 2024 18:38
Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

I feel unsure about introducing some of these variables. Can you describe in more detail the problems each of these variables are solving? The current PR description is unhelpful.

packages/itwinui-css/src/utils/icon.scss Outdated Show resolved Hide resolved
packages/itwinui-css/src/radio-tile/radio-tile.scss Outdated Show resolved Hide resolved
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.

2 participants