carbon-design-system / carbon-website

The website for the Carbon Design System.
https://www.carbondesignsystem.com
Apache License 2.0
278 stars 778 forks source link

Fix: Broken links and incorrect notification color tokens #4282

Open Kritvi-bhatia17 opened 1 week ago

Kritvi-bhatia17 commented 1 week ago

Closes #4187 Closes #4244

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

Changelog

Changed

vercel[bot] commented 1 week ago

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 commented 1 week ago

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

Kritvi-bhatia17 commented 6 days ago

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 commented 6 days ago

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 commented 6 days ago

@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.

Kritvi-bhatia17 commented 5 days ago

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 commented 5 days ago

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