[WIP] Add icon parameter to st.expander
#8716
Conversation
| <StyledDynamicIcon iconValue={icon} {...props}> | ||
| <MaterialFontIcon pack={pack} iconName={icon} {...props} /> | ||
| </StyledDynamicIcon> | ||
| ) | ||
| case "emoji": | ||
| default: | ||
| return <EmojiIcon {...props}>{icon}</EmojiIcon> | ||
| return ( | ||
| <StyledDynamicIcon iconValue={icon} {...props}> |
There was a problem hiding this comment.
I think it might be good to add data-testid to both of these options, e.g.: stIconEmoji and stIconMaterial and use these test ids in the tests. Or maybe we actually set the testid from DynamicIconProps here, at the moment it seems to be unused, or?
There was a problem hiding this comment.
I've added data-testids for stIconEmoji and stIconMaterial and used these in the tests 👍
But I also did the following and not sure the last part is needed:
- For st.status's check and error icons, we used to add
data-testid:stExpanderIconCheckandstExpanderIconError. I continue passing them to<DynamicIcon> - When the icon value is neither check nor error, I added a generic data-testid
stExpanderIcon. Not sure if this is really needed or if I should remove it and not specify one.
There was a problem hiding this comment.
Hmm, I think both solutions would be fine... but I'm slightly favoring removing this distinction and only have one test id to have lower tech debt. I think the related unit tests aren't that important since we are anyways testing this via e2e snapshot tests as well.
| * If the icon is "check", it will render a check icon. | ||
| * If the icon is "error", it will render an error icon. |
There was a problem hiding this comment.
nit: I think these two comments can be removed with the recent change
| <DynamicIcon | ||
| color="inherit" | ||
| iconValue={icon} | ||
| aria-hidden={statusIconTestIds[icon] ? "true" : undefined} |
There was a problem hiding this comment.
nit: I don't think we have to differentiate here, I think we can even just remove the aria-hidden completely since the new icons are probably more accessible since they actually contain descriptive text.
| <StyledDynamicIcon iconValue={icon} {...props}> | ||
| <MaterialFontIcon pack={pack} iconName={icon} {...props} /> | ||
| </StyledDynamicIcon> | ||
| ) | ||
| case "emoji": | ||
| default: | ||
| return <EmojiIcon {...props}>{icon}</EmojiIcon> | ||
| return ( | ||
| <StyledDynamicIcon iconValue={icon} {...props}> |
There was a problem hiding this comment.
Hmm, I think both solutions would be fine... but I'm slightly favoring removing this distinction and only have one test id to have lower tech debt. I think the related unit tests aren't that important since we are anyways testing this via e2e snapshot tests as well.
| iconValue: string | ||
| size?: IconSize | ||
| margin?: string | ||
| padding?: string | ||
| testid?: string | ||
| color?: ThemeColor |
There was a problem hiding this comment.
I think we can remove a couple of properties here that are not used for styling: iconValue and testid
| color="inherit" | ||
| iconValue={icon} | ||
| aria-hidden={statusIconTestIds[icon] ? "true" : undefined} | ||
| data-testid={statusIconTestIds[icon] || "stExpanderIcon"} |
There was a problem hiding this comment.
It think we better should use the testid prop defined in DynamicIcon here instead. Using other props here is working since we are routing it through to the styled component, but that's a very unusual pattern (maybe we should even remove this part). It is better torelyy on the props that are explicitly defined in DynamicIcon.
Describe your changes
Adds an
iconkwarg tost.expanderto optionally support single-character emojis or material icons.iconproto fieldst.statuswithDynamicIconmaterial iconsGitHub Issue Link (if applicable)
N/A
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.