chore(DataList): added RTL modifier class to icon in demo #9605
Conversation
|
Preview: https://patternfly-react-pr-9605.surge.sh A11y report: https://patternfly-react-pr-9605-a11y.surge.sh |
| aria-label={this.state.allExpanded ? 'Collapse all rows' : 'Expand all rows'} | ||
| > | ||
| <ToolbarExpandIconWrapper> | ||
| <ToolbarExpandIconWrapper className="pf-v5-m-mirror-inline-rtl"> |
There was a problem hiding this comment.
You could be able to update this to use the prop now that your other PR went in
There was a problem hiding this comment.
That would just require using our Icon component here as well. Since this is a demo may not be a big deal so I can do that if that sounds good to you
thatblindgeye
force-pushed
the
iss9591
branch
from
49fbaad to
abdec44
Compare
|
@tlabaj @mcoker Latest update show the Icon component wrapped around the ToolbarExpandIconWrapper, as doing it the other way (Icon inside of ToolbarExpandIconWrapper) resulted in the caret pointing up instead of down in the expanded state:
I imagine because Icon itself doesn't have any styling dealing with the rotation of an icon, whereas |
| <Icon shouldMirrorRTL> | ||
| <ToolbarExpandIconWrapper> | ||
| <AngleRightIcon /> | ||
| </ToolbarExpandIconWrapper> | ||
| </Icon> |
There was a problem hiding this comment.
Oh looks like this is something we missed in core. I put up a PR to fix it here - patternfly/patternfly#5937
There was a problem hiding this comment.
Ah yeah I was considering putting a core PR up as well, but since we're not hardcoding the icon i ended up going with the example update instead. If we'd want to go the CSS route we may want to consider hardcoding this icon rather than allowing a consumer to pass any icon they want.
There was a problem hiding this comment.
Ah! I wasn't keeping in mind that the icon is passed explicitly. I think we should definitely provide the icon the same way we do with the data-list and table, which is the primary use case for this toggle in the toolbar
Though I'm assuming that's a breaking change? If so I'll back the change out in core, use the icon component like you have here, and we can create a breaking change issue to update that later?
There was a problem hiding this comment.
@tlabaj wdyt? Would make sense to use the same icon used most other places for expansion here.
There was a problem hiding this comment.
Yes, rendering the icon internally would be breaking since they can technically pass in whatever icon they want right now.
5 participants
What: Closes #9591
Additional issues: