Skip to content

Conversation

@sreidthomas
Copy link
Contributor

@sreidthomas sreidthomas commented Mar 14, 2025

This PR is for updating the Icon Component that exists in the experimental by:

  • Moving Icon from experimental to stable folder
  • Adding bootstrap icons and the backwards compatibility use of font awesome (some icons do not have bootstrap icons)
  • Adding new name / label attributes
  • Keeping the old data-icon, data-size and is-highlighted attributes for backwards compatibility
  • The color and size of the icon should be determined by the color and font-size style properties (either applied directly to the icon element or its parent). See an example here: https://shoelace.style/components/icon#colors

@maxatdetroit maxatdetroit linked an issue Mar 14, 2025 that may be closed by this pull request
@maxatdetroit maxatdetroit removed their request for review March 19, 2025 15:32
@maxatdetroit
Copy link
Contributor

@sreidthomas I removed myself from review for now since I think you're still working on this. When you're ready for review, go ahead and re-add me as a reviewer.

Copy link
Contributor

@maxatdetroit maxatdetroit left a comment

Choose a reason for hiding this comment

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

In addition to the inline comments below, how are you testing this? If you're testing manually using storybook, then would you please list the scenarios you tested on the PR and include a video of the testing?

Note: There are a lot of scenarios to test given the scope of changes here. Make sure to test all the new combinations. E.g.

  • data-size and lightDOM font-size are set; what happens? only one is set; what happens? etc.
  • data-icon and name attribute are set; what happens? only one is set; what happens? etc.
  • ... same thing with library attribute.

Also, please see best practices (4) and (6) in #201 . We should update the Icon to reflect JS properties from the name attribute and improve our storybook documentation.

Copy link
Contributor

@maxatdetroit maxatdetroit left a comment

Choose a reason for hiding this comment

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

The property implementation needs to be tweaked slightly, otherwise this is on the right track.

@jedgar1mx
Copy link
Member

Need story to review inheritance on both size and color

Copy link
Contributor

@maxatdetroit maxatdetroit left a comment

Choose a reason for hiding this comment

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

When I try to view the inheritance stories for testing I'm receiving undefined errors in Storybook. Is this just on my end, or are you seeing the same issues @sreidthomas?

image

I also left some comments to fix before we merge this in.

@sreidthomas
Copy link
Contributor Author

When I try to view the inheritance stories for testing I'm receiving undefined errors in Storybook. Is this just on my end, or are you seeing the same issues @sreidthomas?

image

I also left some comments to fix before we merge this in.
I am getting the same. I added this update as per Edgar's review and I have not been sure how to get it to work without the icons being undefined.

@sreidthomas
Copy link
Contributor Author

When I try to view the inheritance stories for testing I'm receiving undefined errors in Storybook. Is this just on my end, or are you seeing the same issues @sreidthomas?
image
I also left some comments to fix before we merge this in.
I am getting the same. I added this update as per Edgar's review and I have not been sure how to get it to work without the icons being undefined.

THIS IS FIXED

Copy link
Contributor

@maxatdetroit maxatdetroit left a comment

Choose a reason for hiding this comment

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

It looks like the color is being inherited but not the size. Notice in the video that 48px size and 64px have the same sized icon. Do you know what the issue is? Would you please see if you can fix it? Let me know if you want a hand.

Screen.Recording.2025-05-06.102319.mp4

@sreidthomas
Copy link
Contributor Author

It looks like the color is being inherited but not the size. Notice in the video that 48px size and 64px have the same sized icon. Do you know what the issue is? Would you please see if you can fix it? Let me know if you want a hand.
Screen.Recording.2025-05-06.102319.mp4

Yeah I'm not sure if you remember but we spoke about this and I couldn't figure out why the size was not being inherited only the color. I did ask for help with this @maxatdetroit

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.

Update icon component

4 participants