Conversation
|
Preview: https://patternfly-react-pr-10019.surge.sh A11y report: https://patternfly-react-pr-10019-a11y.surge.sh |
thatblindgeye
left a comment
There was a problem hiding this comment.
LGTM. None of this is blocking for this PR, but I'd think that we should allow icon placement in React similar to Core. However, I'm wondering if we should continue to have the icon and text be prop based vs just allowing a consumer to pass whatever they want as children in whatever order. So instead of:
<ToggleGroupItem icon={...} text="..." iconPosition="..." />
Make it:
<ToggleGroupItem>
...either icon followed by text, or text followed by icon
</ToggleGroupItem>
@mcoker looks like the only reason we're using the .pf-v5-c-toggle-group__[icon/text] classes is to add margin-inline-start styling. Which in React, whatever gets passed to the icon and text props are wrapped in that __[icon/text] element. Other than providing a unique container for the icon and text, is there another use case, or is just adding a gap to .pf-v5-c-toggle-group__button something we should consider (instead of using these containers to add margin-inline-start styling)? That may mess up styling if what someone passes to .pf-v5-c-toggle-group__text is something like, <span class="fancy-text">Fancy</span><span>Text</span>, but that would still be fixable by the consumer adding their own wrapper.
This could also be a discussion for other components like Button that utilize icon and iconPosition props, so could be worth adding an iconPosition prop to ToggleGroupItem as a temporary soluation and having a followup regarding convo regarding all of this.
wise-king-sullyman
left a comment
There was a problem hiding this comment.
React code wise I think we're good, it does look like the selected color is different than what is in core/figma, not sure what the deal is there.
thatblindgeye
left a comment
There was a problem hiding this comment.
css wise this looks good. I pulled the branch in locally and rebased and the styling matches Core. This PR might just be built with too early an alpha version from Core.
|
@thatblindgeye sorry for the delay! Pretty much anywhere we support an icon as a structural part of a component, we'll have some kind of container for the icon. That allows us to apply specific styles to the icon, like the margin you mentioned, but also other styles like color, size, position (without the need for end user/react changes, using something like the And the text container allows the consumer to pass anything they want as the content (like your example If we just let the user pass children to the button and we relied on either a plain text node, or text within a wrapper, for the text + icon, a couple of things come to mind:
Though I do think |
|
@mcoker thanks for the response! Yeah, there are definitely some downsides to updating the API to passing children (rather than props) for icon/text placement, and something like that would have to be done across other components. iconPosition sounds like the best solution right now/in general then, just set it up similar to Button, and easier in terms of making sure whatever icon or text content is rendered inside the appropriate wrapper. We can either open a followup to add an |
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #9982
No token update was necessary.
QUESTION: in HTML examples https://pf6.patternfly.org/components/toggle-group#icon-and-text there is also a variant with icon after text.
We currently don't support that in React - is it something we want?
(I checked design guidelines and none of the examples shows the icon after text)