feat(pagination): add disabled variation#2015
Conversation
|
Deploy preview for pf-next ready! Built with commit 0476188 |
mattnolting
left a comment
mattnolting
left a comment
There was a problem hiding this comment.
Looks like you need to add pf-m-disabled to your next/last buttons. Other than that, lgtm
|
Thanks @mattnolting updated!! |
|
@christiemolloy looks like in #1973, we missed adding And in this PR, we need to add
|
| {{> pagination-options-menu id="pagination-options-menu-top-disabled-example" options-menu--IsText="true" options-menu-toggle--IsDisabled="true"}} | ||
|
|
||
| {{#> pagination-nav}} | ||
| {{#> button button--modifier="pf-m-plain pf-m-disabled" button--attribute='aria-label="Go to first page" aria-disabled="true"'}} |
There was a problem hiding this comment.
Looks like in the pagination component, we're using pf-m-disabled wrong. We should be using disabled on buttons and pf-m-disabled on a elements. So these buttons need to be updated to use disabled
Do you want to do that as part of this issue, or a follow up issue?
There was a problem hiding this comment.
I can do it in this issue
|
@mcoker updated... Added disabled variation to the plain options menu with text. I also added
|
| @@ -6,7 +6,7 @@ | |||
| | -- | -- | -- | | |||
There was a problem hiding this comment.
@mcoker @jgiardino now that the react implementation is complete should I create an issue to fill in the options menu docs? or do you know if this is still on hold?
|
|
||
| {{#> pagination-nav}} | ||
| {{#> button button--modifier="pf-m-plain pf-m-disabled" button--attribute='aria-label="Go to first page" aria-disabled="true"'}} | ||
| {{#> button button--modifier="pf-m-plain" button--attribute='disabled aria-label="Go to first page" aria-disabled="true"'}} |
There was a problem hiding this comment.
I don't think we need aria-disabled, either, if the element has the disabled attribute. @jgiardino can you confirm?
There was a problem hiding this comment.
In this article it says: "A button with disabled as well as the button with aria-disabled will be labeled as "dimmed button" https://a11y-101.com/development/aria-disabled
There was a problem hiding this comment.
Looks like we only need "disabled"
|
On the "Options menu - disabled" and "Options menu - plain |
|
thanks @mcoker updated! |
| &:disabled { | ||
| pointer-events: none; | ||
|
|
||
| &:not(.pf-m-plain) { |
There was a problem hiding this comment.
Removing this gives a background to the plain (non .pf-m-text) button.
I also don't know that you need the &:not(.pf-m-plain) on L179, either. It's correct code-wise, but seems unnecessary and increases specificity.
What if you rewrote this block to
&.pf-m-disabled,
&:disabled {
pointer-events: none;
&:not(.pf-m-plain),
&.pf-m-text {
--pf-c-options-menu__toggle--Background: var(--pf-c-options-menu__toggle--disabled--BackgroundColor);
}
&::before {
border: 0;
}
}
|
I'm good with that change! updated! @mcoker |
| | `role` or `aria` | `pf-c-options-menu` | accessibility notes. | | ||
| *Note:* The attribute `aria-selected="true"` should be set programmatically to the selected item(s). | ||
|
|
||
| | `disabled` | `.pf-c-options-menu__toggle`, `.pf-c-options-menu__toggle-button` | Disables the options menu toggle and toggle button and removes it from keyboard focus. | |
There was a problem hiding this comment.
I think you want to move this above the *Note:*
|
🎉 This PR is included in version 2.20.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
closes #1963
Adds disabled variation for the top pagination component - assuming that we don't need to also show an example for the bottom variation?