Skip to content

chore(DataList): added RTL modifier class to icon in demo #9605

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tlabaj merged 2 commits into from
Sep 19, 2023
Merged

chore(DataList): added RTL modifier class to icon in demo #9605

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9de4a6
chore(DataList): added RTL modifier class to icon in demo
Sep 12, 2023
abdec44
Updated to use new Icon shouldMirrorRTL prop
Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions packages/react-core/src/demos/DataListDemo.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ import {
ToolbarExpandIconWrapper,
ToolbarContent,
SearchInput,
Tooltip
Tooltip,
Icon
} from '@patternfly/react-core';
import CodeBranchIcon from '@patternfly/react-icons/dist/esm/icons/code-branch-icon';
import AngleRightIcon from '@patternfly/react-icons/dist/esm/icons/angle-right-icon';
Expand Down Expand Up @@ -123,9 +124,11 @@ class ExpandableDataList extends React.Component {
variant="plain"
aria-label={this.state.allExpanded ? 'Collapse all rows' : 'Expand all rows'}
>
<ToolbarExpandIconWrapper>
<AngleRightIcon />
</ToolbarExpandIconWrapper>
<Icon shouldMirrorRTL>
<ToolbarExpandIconWrapper>
<AngleRightIcon />
</ToolbarExpandIconWrapper>
</Icon>
Comment on lines +127 to +131
Copy link
Contributor

@mcoker mcoker Sep 15, 2023

Choose a reason for hiding this comment

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

Oh looks like this is something we missed in core. I put up a PR to fix it here - patternfly/patternfly#5937

Copy link
Contributor Author

@thatblindgeye thatblindgeye Sep 15, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@mcoker mcoker Sep 15, 2023

Choose a reason for hiding this comment

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

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

patternfly-react/packages/react-core/src/components/DataList/DataListToggle.tsx

Line 48 in dae070a

<AngleRightIcon />

patternfly-react/packages/react-table/src/components/Table/utils/decorators/treeRow.tsx

Line 58 in dae070a

<AngleDownIcon aria-hidden="true" />

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?

Copy link
Contributor Author

@thatblindgeye thatblindgeye Sep 15, 2023

Choose a reason for hiding this comment

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

@tlabaj wdyt? Would make sense to use the same icon used most other places for expansion here.

Copy link
Contributor

@tlabaj tlabaj Sep 18, 2023

Choose a reason for hiding this comment

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

Yes, rendering the icon internally would be breaking since they can technically pass in whatever icon they want right now.

</Button>
</Tooltip>
</ToolbarItem>
Expand Down