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

fix(code-snippet): use transparent color instead of transparent keyword for linear gradients #4770

Closed
wants to merge 5 commits into from
Closed

fix(code-snippet): use transparent color instead of transparent keyword for linear gradients #4770

wants to merge 5 commits into from

Conversation

jendowns
Copy link
Contributor

@jendowns jendowns commented Nov 25, 2019

Closes #4769

Safari interprets transparent (which maps to rgba(0, 0, 0, 0)) differently & therefore the linear gradients used in the CodeSnippet component are darker, like a transparent black.

Here a couple links explaining this:

Changelog

Changed

  • use the Sass rgba() color function to make primary gradient color transparent, rather than using transparent keyword

@jendowns jendowns requested a review from a team as a code owner November 25, 2019 19:49
@ghost ghost requested review from asudoh and emyarod November 25, 2019 19:49
@jendowns jendowns changed the title fix(code-snippet): make color tokens transparent for linear-gradient fix(code-snippet): use transparent color instead of transparent keyword for linear gradients Nov 25, 2019
@netlify
Copy link

netlify bot commented Nov 25, 2019

Deploy preview for the-carbon-components ready!

Built with commit 283cfe0

https://deploy-preview-4770--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Nov 25, 2019

Deploy preview for carbon-components-react ready!

Built with commit 283cfe0

https://deploy-preview-4770--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Nov 25, 2019

Deploy preview for carbon-elements ready!

Built with commit 283cfe0

https://deploy-preview-4770--carbon-elements.netlify.com

@jendowns
Copy link
Contributor Author

Hey FYI if you notice a strange style regression with the CopyButton inside the CodeSnippet, that appears to be caused by a class name getting left out in an earlier change.

I reported the issue here: #4772 & I'm working on a fix for that separately 👍

@netlify
Copy link

netlify bot commented Nov 25, 2019

Deploy preview for the-carbon-components ready!

Built with commit 98e7979

https://deploy-preview-4770--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Nov 25, 2019

Deploy preview for carbon-elements ready!

Built with commit 98e7979

https://deploy-preview-4770--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Nov 25, 2019

Deploy preview for carbon-components-react ready!

Built with commit 98e7979

https://deploy-preview-4770--carbon-components-react.netlify.com

@abbeyhrt
Copy link
Contributor

Hi @jendowns! For me, the linear-gradient isn't there anymore. Here's what I'm seeing now:
Screen Shot 2019-11-25 at 1 43 13 PM

@jendowns
Copy link
Contributor Author

jendowns commented Nov 25, 2019

Thanks @abbeyhrt!

I can see the gradients locally, but not in these deployments (like you just reported).

Looks like the reason is related to this issue I opened a couple weeks ago: #4345

Using the rgba() Sass function with color tokens is fine locally but then the deployments throw in CSS variables & things are compiled incorrectly, with the variables? 😭

// Original Sass
background-image: linear-gradient(to right, rgba($ui-01, 0), $ui-01);

// Compiled *local* CSS, which looks great 🎉
background-image: linear-gradient(to right, rgba(244, 244, 244, 0), #f4f4f4);

// Compiled deployed CSS with variables, which is broken 😭 
background-image: linear-gradient(to right, rgba(var(--cds-ui-01, #f4f4f4), 0), var(--cds-ui-01, #f4f4f4));

@emyarod What do you think should be done here? Is this related to any work you are currently doing for #4345?

I was thinking a possible solution here would be to use the theme mixins & just target each theme... and then manually set these values to rgb, instead of using color tokens. To avoid the variables messing up the gradients in these deployments. 🤔

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me, the gradients are no longer present locally and in the deploy preview

@emyarod
Copy link
Member

emyarod commented Nov 26, 2019

regarding #4345, it's on hold at the moment since we haven't come up with a good way to proceed yet and it should only be affecting experimental code (including the deploy preview) IIRC

@jendowns
Copy link
Contributor Author

Thanks @emyarod! When you say "the gradients are no longer present locally" -- do you mean when you pulled down my branch & ran the storybook environment, you did not see them? (My observation is that they aren't visible on the deploy preview due to variable usage but are visible if running locally)

@emyarod
Copy link
Member

emyarod commented Nov 27, 2019

do you mean when you pulled down my branch & ran the storybook environment, you did not see them?

yes that's what I meant, I don't have the gradient locally with and without custom properties enabled

@jendowns
Copy link
Contributor Author

@asudoh should I close this out given your PR #4781?

@asudoh
Copy link
Contributor

asudoh commented Nov 27, 2019

Yeah makes sense - Considered regular linear-gradient approach but I saw a problem with CSS custom properties. Thank you for trying to address this problem @jendowns!

@jendowns
Copy link
Contributor Author

Thanks @asudoh! In the future please let me know if you'd like me to adjust my existing PR -- always happy to take feedback. 😄

Closing this one out

@jendowns jendowns closed this Nov 27, 2019
@asudoh
Copy link
Contributor

asudoh commented Nov 28, 2019

@jendowns My apologies if it looked like I took over your work - I always recognize you are making great contributions to Carbon. This specific case I got a similar issue on Modal which needed a quick solution. I'll be extra careful if any of my work may conflict with your work and make sure it's communicated. Thank you for your contributions as always!

@jendowns
Copy link
Contributor Author

jendowns commented Dec 2, 2019

No worries at all @asudoh! And thank you so much for your kind words. ❤️

@jendowns jendowns deleted the 4769_code-snippet-safari-gradient branch December 2, 2019 16:54
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.

Code snippet fade effect is inconsistent in Safari
4 participants