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(Themes): add in G80 theme #8478

Merged
merged 7 commits into from
Apr 27, 2021
Merged

Conversation

tw15egan
Copy link
Collaborator

Closes #8471

Adds in the g80 theme for inline theming efforts

Changelog

New

  • g80 is now available as a theme.

Changed

  • Added g80 to the theme switcher in deploy previews

Testing / Reviewing

@aagonzales I didn't see a token for linkPrimaryHover, I think that's the only one missing

@netlify
Copy link

netlify bot commented Apr 21, 2021

Deploy preview for carbon-elements ready!

Built with commit b970a25

https://deploy-preview-8478--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Apr 21, 2021

Deploy preview for carbon-components-react ready!

Built with commit b970a25

https://deploy-preview-8478--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Apr 21, 2021

Deploy preview for carbon-elements ready!

Built with commit a02f4f9

https://deploy-preview-8478--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Apr 21, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit a02f4f9

https://deploy-preview-8478--carbon-components-react.netlify.app

@aagonzales
Copy link
Member

@tw15egan Some of the components aren't rendering correctly because its tokens haven't been updated yet. Should we have the v10 tokens values defined for Gray 80? Or will that not matter once we update everything.

@tw15egan
Copy link
Collaborator Author

@aagonzales yeah so I added this one in the opposite way of the other theme updates with new tokens in that I set the new token value to the IDL color and referenced the old tokens to the new ones.

Will g80 be available as a theme in v10? If not, then we probably don't need to add them if we don't expose the theme on the website or anything

@aagonzales
Copy link
Member

aagonzales commented Apr 22, 2021

No its irrelevant in v10.

oh I see now, I hadn't gotten all the way to the bottom. Something with the selected states and some of the hovers is doing something weird. Let me see if I can figure out what is now that I know those bits

Copy link
Contributor

@jnm2377 jnm2377 left a comment

Choose a reason for hiding this comment

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

🔥

packages/themes/src/g80.js Outdated Show resolved Hide resolved
packages/themes/src/g80.js Show resolved Hide resolved
packages/themes/src/g80.js Outdated Show resolved Hide resolved
packages/themes/src/g80.js Outdated Show resolved Hide resolved
packages/themes/src/g80.js Outdated Show resolved Hide resolved
packages/themes/src/g80.js Outdated Show resolved Hide resolved
packages/themes/src/g80.js Outdated Show resolved Hide resolved
packages/themes/src/g80.js Outdated Show resolved Hide resolved
packages/themes/src/g80.js Outdated Show resolved Hide resolved
packages/themes/src/g80.js Outdated Show resolved Hide resolved
@aagonzales
Copy link
Member

aagonzales commented Apr 26, 2021

Something still isnt rendering correctly with the selected states. Select row backgrounds should be a shade lighter ie layer-selected: G60. I can't tell if its a value in the theme problem or if its just because the old tokens are being used and it will be fine once the new ones come through.

image

@tw15egan
Copy link
Collaborator Author

layerSelected is mapped to gray60, so in the new theme, it will be correct. It could be a mismatch between what we had with the old tokens and what we're mapping them to for backwards compatibility for now, I see thatselectedUI is mapped to backgroundSelected (gray70). Would mapping selectedUI to layerSelected possibly fix this?

@aagonzales
Copy link
Member

Ok lets just leave it alone then. I wont look too hard at the components until they've all been updated with the new tokens.

@tw15egan
Copy link
Collaborator Author

tw15egan commented Apr 26, 2021

@aagonzales I turned on the feature flag so it should all be using the new tokens in all components except the ones in this PR: #8427 (once the deploy is updated)

I'll turn it off before merging but should be the best way to test 👍🏻

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Thanks TJ! That feature flagged really helped. Just one change I found.

packages/themes/src/g80.js Outdated Show resolved Hide resolved
@tw15egan
Copy link
Collaborator Author

@aagonzales updated 👍🏻

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

🎉 cool! looks good now!

@kodiakhq kodiakhq bot merged commit 0a9703d into carbon-design-system:main Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[element] add gray 80 theme
4 participants