fix(Pagination): make items per page text in toggle clickable #7192
Conversation
|
Preview: https://patternfly-react-pr-7192.surge.sh A11y report: https://patternfly-react-pr-7192-a11y.surge.sh |
thatblindgeye
force-pushed
the
pagination_toggle
branch
from
45f12ab to
d224744
Compare
mcarrano
left a comment
mcarrano
left a comment
There was a problem hiding this comment.
This is great. Thanks @thatblindgeye !
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
@tlabaj just checking whether you think changing the OptionsMenu with a Dropdown in the Pagination constitutes a breaking change? Since our tests had to be updated, I wonder if products' tests might end up in a similar situation?
kmcfaul
left a comment
kmcfaul
left a comment
There was a problem hiding this comment.
If the tests reference an id it may not be breaking but if it uses a selector routing through the options menu then the tests may break. I don't know if tests having an issue constitutes a breaking change though.
|
Yes, this would constitute a breaking change. Products may also be using the selector to apply custom styling. |
1 similar comment
|
Yes, this would constitute a breaking change. Products may also be using the selector to apply custom styling. |
| <> | ||
| {showToggle && ( | ||
| <React.Fragment> | ||
| <span className={css(styles.optionsMenuToggleText)}> |
There was a problem hiding this comment.
Products could be applying custom styles here . This would be a breaking change.
thatblindgeye
force-pushed
the
pagination_toggle
branch
from
d224744 to
722b88b
Compare
packages/react-core/src/components/Pagination/OptionsToggle.tsx
Outdated
Show resolved
Hide resolved
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
LGTM! Tho I think we should change the component prop on <Pagination> to be something that refers to the menu.
Also assuming we continue to bundle this menu with the pagination component, and we keep the options menu over using a menu-toggle/menu pair, should we make a note in a breaking change release to just use the <OptionsMenu> component? It looks like we're reconstructing parts of it in this component. I could be reading it wrong though.
| /** The type or title of the items being paginated */ | ||
| itemsTitle?: string; | ||
| /** The text to be displayed on the Options Toggle */ | ||
| /** Accessible label for the Options Toggle */ |
There was a problem hiding this comment.
I just spot checked a handful of other react component props and we use the "accessible label" wording to describe the function of a [whatever]AriaLabel prop. This may be OK, just wanted to mention it.
There was a problem hiding this comment.
That's a fair point. My main reason for changing the description was due to "text to be displayed" didn't seem accurate either, as it seemed like the prop was only being used to add an aria-label to that dropdown toggle.
thatblindgeye
force-pushed
the
pagination_toggle
branch
from
c65c052 to
830b583
Compare
7 participants
What: Closes #7123
Currently this update changes the outer element of the toggle from a
divto abutton, and placing class names where appropriate. I also added in thearia-haspopupprop to match the current Core workspace.This can also be updated to be more of an opt-in where a prop can be passed in to decide whether the entire toggle text should be clickable (button) or whether it should behave as it currently does in React where only the icon is clickable (div).
Pagination preview build for convenience.
Additional issues: