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: Fix colorHex to handle alpha values #1441

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

Conversation

laxa88
Copy link

@laxa88 laxa88 commented Feb 3, 2025

Issue #, if available:

Description of changes:

By default, when using the js transformGroup, all tokens are converted into hex values (for example, #000000) before additional transforms are applied. For rgba tokens, this causes the color values to lose the transparency value, resulting in all tokens to default to #xxxxxxFF (full opacity).

This PR fixes the default transform to use hex8 to account for RGBA transforms.

I'm not entirely sure if there is a historical reason for using colorHex instead of colorHex8 for js transforms, but if this is not an oversight, I would appreciate if someone can share some details. Otherwise, I hope this PR helps. 🙇

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@laxa88 laxa88 requested a review from a team as a code owner February 3, 2025 05:54
@laxa88 laxa88 changed the title default convert tokens to hex8 to account for rgba colors Fix: Default convert tokens to hex8 to account for rgba colors Feb 3, 2025
@laxa88 laxa88 changed the title Fix: Default convert tokens to hex8 to account for rgba colors Fix: Default js transform to convert color to hex8 instead of hex Feb 3, 2025
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1441.d16eby4ekpss5y.amplifyapp.com

@laxa88 laxa88 force-pushed the fix-default-js-transform-to-hex8 branch from 3ce1ed3 to edabe47 Compare February 3, 2025 06:02
@jorenbroekema
Copy link
Collaborator

Hi, I think this is mostly an oversight to be honest, I actually would prefer if we didn't have 2 transforms in the first place and just have 1 that handles both hex8 but also uses hex6 when the alpha channel is set 1.
So if possible, can we adjust the regular hex transform to handle hex8 when necessary and deprecate the hex8 specific transform, so we can remove it in the next breaking change but start telling users to use the regular hex one instead?

And can we add some tests to the PR?

@laxa88
Copy link
Author

laxa88 commented Feb 4, 2025

@jorenbroekema Thank you for the info and feedback.

  • Reverted the change to colorHex8
  • Updated the colorHex logic to handle hex6 and hex8, similar to colorCss
  • Added and updated colorHex tests

@laxa88 laxa88 changed the title Fix: Default js transform to convert color to hex8 instead of hex Fix: Fix colorHex to handle alpha values Feb 4, 2025
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