-
Notifications
You must be signed in to change notification settings - Fork 201
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: adds support to parse "transparent" token references #3452
base: main
Are you sure you want to change the base?
chore: adds support to parse "transparent" token references #3452
Conversation
|
🚀 Deployed on https://pr-3452--spectrum-css.netlify.app |
File metricsSummaryTotal size: 2.25 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
File change detailsbutton
tokens
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
f9665ba
to
7820d75
Compare
7820d75
to
892feda
Compare
7429c49
to
5d1daf9
Compare
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.
It seems like there's a lot of great overlap here with the transparent mapping and the rgb-mapping. What are your thoughts on adding that functionality directly to the rgb-mapping plugin rather than building a new one?
079d274
to
6938024
Compare
c68f4e3
to
d2272ea
Compare
b4310e6
to
9602337
Compare
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.
Nice work on this. I did a diff on the tokens index CSS and it looks like what is expected.
I just had a few questions/comments, which shouldn't be blocking.
Will we need another PR after this releases in order to do a new release of our tokens package using the new @spectrum-tools/postcss-rgb-mapping
version? Unless there is a way for changesets to handle it at the same time.
--spectrum-transparent-white-100-rgb: 100, 100, 100; | ||
--spectrum-transparent-white-100-opacity: 0.5; | ||
--spectrum-transparent-white-100: rgba(var(--spectrum-transparent-white-100-rgb), var(--spectrum-transparent-white-100-opacity)); |
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.
Should this be using the rgb
syntax without commas, like the other existing values above?
Similarly, should the rgba()
be using the slash syntax? Or would that fail the tests? I'm wondering why some are different.
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 believe it fails the test. I think it's because in the fixtures/basic.css
, I already defined the -rgb
and -opacity
custom props, with the commas. Sort of like I already did the work for part of the modern CSS test.
Any ideas if I can fix that somehow?
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.
How about something like this? 51481bd
Looks like it passes!
--spectrum-transparent-white-100-rgb: 100, 100, 100; | ||
--spectrum-transparent-white-100-opacity: 0.5; | ||
--spectrum-transparent-white-100: rgba( | ||
var(--spectrum-transparent-white-100-rgb), | ||
var(--spectrum-transparent-white-100-opacity) | ||
); | ||
--disabled-static-white-background-color-rgb: var( | ||
--spectrum-transparent-white-100-rgb | ||
); | ||
--disabled-static-white-background-color-opacity: var( | ||
--spectrum-transparent-white-100-opacity | ||
); | ||
--disabled-static-white-background-color: rgba( | ||
var(--disabled-static-white-background-color-rgb), | ||
var(--disabled-static-white-background-color-opacity) | ||
); |
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 think that separating the two chunks of example code with CSS comments (e.g. /* Example 2: Remapping of transparent tokens */
and /* Example 1: RGB and opacity postfixed variables */
) explaining what it does would be helpful in trying to see the changes. Since the before/after code is a lot longer now, it's a little less clear trying to compare and look for differences. Just a thought.
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 like that idea. I feel like I struggled a little bit to be concise with the CSS comments. Let me know if I can clarify anything. 6147109
- postcss-transparent-mapping pluging created - test.js file and new fixtures/expected CSS files
- adds transparent-mapping to tokens postcss config - adds transparent-mapping to tokens devDependencies
9602337
to
6147109
Compare
Description
This PR builds upon the idea the the rgb-mapping plugin introduced to the repo.
--spectrum-disabled-content-color: var(--spectrum-transparent-white-200)
--spectrum-disabled-content-color-rgb
and--spectrum-disabled-content-color-opacity
--spectrum-disabled-content-color-rgb: var(--spectrum-transparent-white-200-rgb
)--spectrum-disabled-content-color: rgba(var(--spectrum-disabled-content-color-rgb), var(--spectrum-disabled-content-color-opacity)
This PR does not refactor any CSS to use the newly created custom properties that should occur with this PR. No VRT diffs should occur.
Jira/Specs
CSS-451
How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation steps
nvm use && git clean -dfX && yarn install && yarn clean && yarn build
for a fresh build and installyarn test:plugins
in your terminal and verify all plugin tests pass, includingpostcss-rgb-mapping
yarn start
to verify the project runs as expected, with no errorsREADME
in theplugins
directory. The new documentation should make sense, be accurate, and be grammar-free.Additional validation
Compare the
dist/global-vars.css
with the newindex.css
that was just generated whenyarn build
&yarn start
were run.In
tokens/dist/global-vars.css
, you should see approximately 15 instances of a custom property's value set to a token like--spectrum-transparent-*
. These are the custom properties this PR is targeting, listed below:dist/global-vars.css
--spectrum-disabled-static-white-background-color
--spectrum-disabled-static-black-background-color
--spectrum-disabled-static-white-content-color
--spectrum-disabled-static-black-content-color
--spectrum-disabled-static-white-border-color
--spectrum-disabled-static-black-border-color
--spectrum-static-black-track-color
--spectrum-static-white-track-color
--spectrum-static-black-track-indicator-color
--spectrum-static-white-track-indicator-color
--spectrum-color-loupe-drop-shadow-color
--spectrum-color-loupe-inner-border
--spectrum-floating-action-button-drop-shadow-color
Additionally, there are component-level properties pointing to a transparent token for light mode and dark mode:
tokens/custom/dark-vars.css
--spectrum-calendar-day-background-color-down
tokens/custom/light-vars.css
--spectrum-calendar-day-background-color-down
Verify that each of the custom properties listed above has new
-rgb
and-opacity
custom properties in thedist/css/index.css
.For instance, you should see something like this in
dist/global-vars.css
:--spectrum-static-black-track-color: var(--spectrum-transparent-black-300);
After the plugin runs, the
dist/css/index.css
should contain additional-rgb
and-opacity
custom properties. Those new properties should have values that reflect the correspondingtransparent-black-300-rgb
and-opacity
props as well, like so:Finally, the value for
--spectrum-static-black-track-color
utilizes the new-rgb
and-opacity
custom properties:Regression testing
Validate:
Screenshots
To-do list