Skip to content

Conversation

didoo
Copy link
Contributor

@didoo didoo commented Jul 30, 2024

📌 Summary

While reviewing #2274, I realized that we don't support pre-defined "foreground" colors for the Hds::Icon, in the same way as we do for the Hds::Text component. This leads to this different way of applying a foreground color:

screenshot_4047

This PR addresses this difference (while the Hds::Icon has not yet been released, so it's just an extension of the existing API, without impact on our consumers).

Notice: I have tested the change in Cloud UI and it worked as expected

🛠️ Detailed description

In this PR I have:

  • added support for predefined “foreground” colors to the @color argument
  • updated “colors” section of the showcase page
  • updated @color in "Component API" section
  • updated “Color” sub-section in “How to use” section

Preview: https://hds-showcase-git-hds-icon-color-argument-hashicorp.vercel.app/components/icon#color

Notice: I haven't added a changelog to this PR because lives on top of the Hds::Icon ones, so the changelog is provided by them.

🔗 Links

📸 Screenshots

screenshot_4025

👀 Component checklist

  • Percy was checked for any visual regression

💬 Please consider using conventional comments when reviewing this PR.

@didoo didoo requested review from a team and zamoore July 30, 2024 12:19
@vercel
Copy link

vercel bot commented Jul 30, 2024

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

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Jul 30, 2024 8:51pm
hds-website ✅ Ready (Inspect) Visit Preview Jul 30, 2024 8:51pm

@hashibot-hds hashibot-hds added packages/components docs-website Content updates to the documentation website showcase labels Jul 30, 2024
@didoo didoo force-pushed the hds-icon-color-argument branch from 88a3417 to 8f9669d Compare July 30, 2024 15:49
@didoo didoo changed the title Hds::Icon - Add support for predefined foreground colors in@color argument Hds::Icon - Add support for predefined "foreground" colors in@color argument Jul 30, 2024
@didoo didoo force-pushed the hds-icon-color-argument branch from 1e4b58e to 8f9669d Compare July 30, 2024 16:56
Copy link
Contributor

@zamoore zamoore 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. Added a minor grammar suggestion, but I'm glad that icon color is in alignment with the rest of the components in the system.

@zamoore zamoore force-pushed the HdsIcon-documentation branch from 8f79082 to 6e2d9c8 Compare July 30, 2024 18:14
@didoo didoo force-pushed the hds-icon-color-argument branch 2 times, most recently from 674c392 to e1f9f46 Compare July 30, 2024 19:39
Base automatically changed from HdsIcon-documentation to FEATURE-BRANCH-HdsIcon July 30, 2024 19:50

<Shw::Text::H2>Color</Shw::Text::H2>

<Shw::Text::H4 @tag="h3">Color inheritance</Shw::Text::H4>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you could just do <Shw::Text::H3>Color inheritance</Shw::Text::H4>

Copy link
Contributor

Choose a reason for hiding this comment

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

Missed that the @tag was changed initially. If you want the visual style of the smaller H4 here for some reason then this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nit: you could just do Shw::Text::H3Color inheritance</Shw::Text::H4>

<Shw::Text::H4 @tag="h3"> means render an <h3> tag with the visual style of a .shw-text-h4


export type HdsIconSizes = `${HdsIconSizeValues}`;

export enum HdsIconColorValues {
Copy link
Contributor

Choose a reason for hiding this comment

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

(question): wouldn't we want all of these to be HdsColorValues anyway? Is there a need to have them specifically in IconColorValues? It seems reasonable that if these are our ColorValues we'd want them available for everywhere that we use these values. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, but I ended up keeping them separate for different reasons:

  1. we may want to add also the "brand" colors for the icon (but not the text, I don't see use cases for that); at the moment thought is not clear how we would do it: have different brand colors, which ones would be the ones that could be used for an icon?
  2. this would require to add a global types.ts file, under packages/components/src/components/hds; not a big deal but maybe something we want to consider a bit more

so my thinking is that we can always do it, it's not something for this specific PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-website Content updates to the documentation website packages/components showcase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants