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: Broken links and incorrect notification color tokens #4282

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

Conversation

Kritvi-bhatia17
Copy link
Contributor

@Kritvi-bhatia17 Kritvi-bhatia17 commented Sep 23, 2024

Closes #4187
Closes #4244

This PR addresses and fixes a few bugs on the website.

Changelog

Changed

  • Fixed a few broken links on the Designing's Get started page.

  • Updated some Notification color tokens that were incorrect:

    In both White and Gray 10 themes:

    Token White theme Gray 10 theme
    $notification-background-warning Yellow 10 (#fcf4d6) Yellow 10 (#fcf4d6)

    In the Gray 90 theme:

    Token Gray 90 theme
    $notification-background-error Gray 80 (#393939)
    $notification-background-success Gray 80 (#393939)
    $notification-background-info Gray 80 (#393939)
    $notification-background-warning Gray 80 (#393939)

Copy link

vercel bot commented Sep 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
carbondesignsystem ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2024 2:44pm

@Kritvi-bhatia17
Copy link
Contributor Author

Kritvi-bhatia17 commented Sep 23, 2024

Hi @aagonzales! Just wanted to double-check the color token values with you, so I’m adding you for review.
Thanks in advance!

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.

This is what I see in code for the White and Gray 10 theme $notification-background-warning Its Yellow 30 and not 10. Where did you get the value Yellow 10 from, I see the other are the 10 step so making sure code isn't the thing that's wrong.

image

@Kritvi-bhatia17
Copy link
Contributor Author

This is what I see in code for the White and Gray 10 theme $notification-background-warning Its Yellow 30 and not 10. Where did you get the value Yellow 10 from, I see the other are the 10 step so making sure code isn't the thing that's wrong.

image

Thanks for bringing that up @aagonzales!
I checked in Figma, and the value showed as Yellow 10, but maybe @alisonjoseph can confirm if there's an overlay involved or if we need to update the values in the code as well?

@alisonjoseph
Copy link
Member

alisonjoseph commented Sep 25, 2024

The hex codes that are being rendered for notification-background-warning
white: #fdf6dd
gray 10: #fdf6dd
gray 90: #393939
gray 100: #262626

This is the code, so there is a mix happening with Yellow 30 and White 0, which matches what is currently showing on the Color page on the website.

$notification-background-warning: (
  fallback:
    color.mix(
      map.get(notification.$color-map, yellow-30),
      map.get(notification.$color-map, white-0),
      15%
    ),
  values: (
    (
      theme: themes.$white,
      value:
        color.mix(
          map.get(notification.$color-map, yellow-30),
          map.get(notification.$color-map, white-0),
          15%
        ),
    ),
    (
      theme: themes.$g10,
      value:
        color.mix(
          map.get(notification.$color-map, yellow-30),
          map.get(notification.$color-map, white-0),
          15%
        ),
    ),
    (
      theme: themes.$g90,
      value: map.get(notification.$notification-background-warning, g-90),
    ),
    (
      theme: themes.$g100,
      value: map.get(notification.$notification-background-warning, g-100),
    ),
  ),
) !default;

I'm not sure the history here why we aren't just using Yellow-10 in code, however it has been this way since v10 as far as I can tell.

@aagonzales
Copy link
Member

@Kritvi-bhatia17 ok it's all coming back to me now with Alison's comment about being that way since v10. Originally IDL didn't have a full yellow palette, which was added later so there wasn't a yellow 10 when this color map solution was made. Can you open an issue to update the value of that token to be yellow 10 and then I think we can keep the yellow 10 value here on this page and the docs will just be a little ahead of code.

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.

Approving to keep the Yellow 10 in for now with a code update coming soon.

@Kritvi-bhatia17
Copy link
Contributor Author

Oh, thanks a lot, @alisonjoseph & @aagonzales!

Sure, Anna, I’ll open an issue and put a 'DO NOT MERGE' tag on this PR until the dev implements the fix in the code.

@Kritvi-bhatia17
Copy link
Contributor Author

This is currently on hold from merging to fix issue #17586 first, ensuring that both the code and the website are updated accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants