fix(ResponsiveActions): Disable kebab when all actions are disabled#928
fix(ResponsiveActions): Disable kebab when all actions are disabled#928rhamilto wants to merge 3 commits intopatternfly:mainfrom
Conversation
Fixes patternfly#927 - Uses OverflowMenuContext to access isBelowBreakpoint state - Kebab disabled state is now responsive to viewport width: - Above breakpoint: disabled if all regular items are disabled - Below breakpoint: disabled if all items (pinned + regular) are disabled - Created ResponsiveActionsDropdown component to access context - Tracks disabled state separately for pinned vs regular items - Added comprehensive test coverage for all scenarios - Fully backward compatible (no breaking changes)
|
@nicolethoen, one more fix for openshift/console#16203. This fix isn't required for openshift/console#16203 since it is not adding actions inside |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Some nitpicks below, not blocking to me to get this in, though
| } | ||
|
|
||
| export const ResponsiveActions: FunctionComponent<ResponsiveActionsProps> = ({ ouiaId = 'ResponsiveActions', breakpoint = 'lg', children, ...props }: ResponsiveActionsProps) => { | ||
| // Inner component that has access to OverflowMenuContext |
There was a problem hiding this comment.
Nitpicks: can we remove a bunch of the comments added in this PR? They aren't totally necessary since the surrounding code will explain what things are doing in this case.
| const [ isOpen, setIsOpen ] = useState(false); | ||
| const { isBelowBreakpoint } = useContext(OverflowMenuContext); | ||
|
|
||
| // Determine if kebab should be disabled based on breakpoint |
There was a problem hiding this comment.
Same as above comment
| // Below breakpoint: pinned items are IN the dropdown, so check all dropdown items | ||
| // Disabled only if both pinned AND regular items exist and are all disabled | ||
| return (pinnedItemsDisabled.length > 0 || regularItemsDisabled.length > 0) && | ||
| (pinnedItemsDisabled.length === 0 || allPinnedDisabled) && | ||
| (regularItemsDisabled.length === 0 || allRegularDisabled); | ||
| } else { | ||
| // Above breakpoint: pinned items are shown as buttons, only check regular items | ||
| // Disabled only if there are regular items and they're all disabled |
There was a problem hiding this comment.
Same as above comment regarding the comments here
| {children} | ||
| </OverflowMenuDropdownItem> | ||
| ); | ||
| // Track disabled state separately for pinned vs regular items |
There was a problem hiding this comment.
Same as above comment
Address PR review feedback by removing unnecessary comments that don't add value beyond what the code already expresses. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Thanks, @thatblindgeye. Claude loves comments. I asked it to remove them. It did not object. :) |
| const { container } = render( | ||
| <ResponsiveActions breakpoint="lg"> | ||
| <ResponsiveAction isDisabled>Disabled action 1</ResponsiveAction> | ||
| <ResponsiveAction isDisabled>Disabled action 2</ResponsiveAction> | ||
| </ResponsiveActions>); | ||
|
|
||
| // Kebab toggle should be disabled when all dropdown items are disabled | ||
| const kebabToggle = container.querySelector('[data-ouia-component-id="ResponsiveActions-menu-dropdown-toggle"]'); | ||
| expect(kebabToggle).toHaveAttribute('disabled'); | ||
| expect(container).toMatchSnapshot(); |
There was a problem hiding this comment.
Could the container.querySelectors be changed to screen.getByRole('button')s to better align with RTL conventions?
Also I think we could probably remove the snapshot tests from these unless there's something about the structure explicitly that we're trying to check, and the associated container destructuring.
| import { Button, Dropdown, DropdownList, MenuToggle, OverflowMenu, OverflowMenuContent, OverflowMenuControl, OverflowMenuDropdownItem, OverflowMenuGroup, OverflowMenuItem, OverflowMenuProps } from '@patternfly/react-core'; | ||
| import { EllipsisVIcon } from '@patternfly/react-icons'; | ||
| import { ResponsiveActionProps } from '../ResponsiveAction'; | ||
| import { OverflowMenuContext } from '@patternfly/react-core/dist/esm/components/OverflowMenu/OverflowMenuContext'; |
There was a problem hiding this comment.
Hm, with this not being something we explicitly export from react-core or document as part of our public API I'm not sure about importing it here. I would definitely be against it in a product, but maybe it's ok to do stuff like that in an extension? WDYT @kmcfaul @thatblindgeye ?
There was a problem hiding this comment.
I will argue it or something like it should be exported. It reduces the need for duplication. For example, I am using it in OpenShift console because to my knowledge, PatternFly doesn't offer a comparable solution where a menu toggle changes based on viewport. Perhaps this is just a gap in PatternFly. But by exposing this, it frees up developers to solve for problems PatternFly doesn't cover using PatternFly tools.
…napshots
Address PR review feedback:
- Replace container.querySelector with screen.getByRole('button') queries
- Remove snapshot tests from disabled state tests (structure is tested by other tests)
- Remove container destructuring where no longer needed
- Use toBeDisabled()/toBeEnabled() instead of toHaveAttribute('disabled')
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
5 participants
Description
Fixes #927
This PR implements the PatternFly design guideline that a kebab menu should be disabled when all of its actions are disabled.
Changes
Implementation
Testing
Behavior
Before
After
Backward Compatibility
✅ Fully backward compatible
Screenshots/Examples
The kebab will now properly show as disabled when appropriate, improving accessibility and UX.
Checklist