Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { noop } from '../../../common/helpers';

export const buttons = [
{ title: 'first', action: { onClick: noop } },
{ title: 'second', action: { href: 'some-url2', 'data-method': 'put' } },
{ title: 'third', action: { onClick: noop } },
{ title: 'first', action: { id: 1, onClick: noop } },
{ title: 'second', action: { id: 2, href: 'some-url2', 'data-method': 'put' } },
{ title: 'third', action: { id: 3, onClick: noop } },
{ title: 'fourth', action: { id: 4, onClick: noop, disabled: true } },
];
Original file line number Diff line number Diff line change
@@ -1,41 +1,95 @@
import React from 'react';
import uuidV1 from 'uuid/v1';
import React, { useState } from 'react';
import PropTypes from 'prop-types';
import { SplitButton, MenuItem, Button } from 'patternfly-react';
import {
Button,
Dropdown,
DropdownItem,
DropdownList,
MenuToggle,
MenuToggleAction,
} from '@patternfly/react-core';
import './actionButtons.scss';

/**
* Generate a button or a dropdown of buttons
* @param {String} title The title of the button for the title and text inside the button
* @param {Object} action action to preform when the button is click can be href with data-method or Onclick
* @return {Function} button component or splitbutton component
* @param {Object} action action to perform when the button is click can be href with data-method or Onclick
* @return {Function} button component or splitbutton with menu toggle action component
*/
export const ActionButtons = ({ buttons }) => {
const [isOpen, setIsOpen] = useState(false);
const onToggleClick = () => setIsOpen(!isOpen);

if (!buttons.length) return null;
if (buttons.length === 1)
return (
<Button bsSize="small" {...buttons[0].action}>
<Button
ouiaId={`action-button-${uuidV1()}`}
size="sm"
variant="primary"
isDisabled={buttons[0].action?.disabled}
{...buttons[0].action}
>
{buttons[0].title}
</Button>
);
const firstButton = buttons.shift();

const [firstButton, ...restButtons] = buttons;

return (
<SplitButton
title={firstButton.title}
{...firstButton.action}
bsSize="small"
<Dropdown
ouiaId={`action-buttons-dropdown-${uuidV1()}`}
isOpen={isOpen}
onOpenChange={openState => setIsOpen(openState)}
toggle={toggleRef => (
<MenuToggle
Copy link
Member

Choose a reason for hiding this comment

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

should MenuToggle support isDisabled as well?

ref={toggleRef}
onClick={onToggleClick}
isExpanded={isOpen}
splitButtonOptions={{
variant: 'action',
items: [
<MenuToggleAction
id={`split-button-action-${uuidV1()}-toggle-button`}
Copy link
Member

Choose a reason for hiding this comment

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

Since I'm not that familiar with uuidV1 or React as a whole, I'd paste here what Claude thinks of it:

uuidV1() is called directly in the render, generating new IDs on every re-render. This can cause:
 - Unnecessary DOM reconciliation
 - Issues with React's diffing algorithm
 - Potential accessibility issues
Consider using useMemo or moving ID generation outside the render path.

I'd say it can be valid issue.

key="split-action"
aria-label={firstButton.title}
>
{firstButton.title}
Copy link
Member

Choose a reason for hiding this comment

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

It seems like the button is missing an action. Maybe onClick={firstButton.action?.onClick} or href handling was forgotten?

</MenuToggleAction>,
],
}}
aria-label="Menu toggle with action split button"
/>
)}
shouldFocusToggleOnSelect
>
{buttons.map(button => (
<MenuItem key={button.title} title={button.title} {...button.action}>
{button.title}
</MenuItem>
))}
</SplitButton>
<DropdownList className="action-buttons">
{restButtons.map(button => (
<DropdownItem
ouiaId={`${button.action?.id}-dropdown-item-id`}
key={`${button.action?.id}-dropdown-item-key`}
title={button.title}
onClick={button.action?.onClick}
Copy link
Member

Choose a reason for hiding this comment

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

If I'm not mistaken, previously MenuItem supported href, but now it's only onClick. Isn't it an issue if href is still used somewhere?

isDisabled={button.action?.disabled}
>
{button.title}
</DropdownItem>
))}
</DropdownList>
</Dropdown>
);
};

ActionButtons.propTypes = {
buttons: PropTypes.arrayOf(
PropTypes.shape({
action: PropTypes.object,
action: PropTypes.shape({
id: PropTypes.number,
onClick: PropTypes.func,
href: PropTypes.string,
disabled: PropTypes.bool,
}),
title: PropTypes.string,
})
),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,44 @@
import { testComponentSnapshotsWithFixtures } from 'foremanReact/common/testHelpers';
import React from 'react';
import { screen, fireEvent, render, act } from '@testing-library/react';
import '@testing-library/jest-dom/extend-expect';

import { ActionButtons } from './ActionButtons';
import { buttons } from './ActionButtons.fixtures';

const fixtures = {
'renders ActionButtons with 0 button': { buttons: [] },
'renders ActionButtons with 1 button': { buttons: [buttons[0]] },
'renders ActionButtons with 3 button': { buttons },
none: { buttons: [] },
one: { buttons: [buttons[0]] },
many: { buttons },
};

describe('ActionButtons', () =>
testComponentSnapshotsWithFixtures(ActionButtons, fixtures));
describe('ActionButtons', () => {
it('renders with 0 buttons', () => {
render(<ActionButtons {...fixtures.none} />);
expect(screen.queryByRole('button')).not.toBeInTheDocument();
});

it('renders 1 button', () => {
render(<ActionButtons {...fixtures.one} />);
expect(screen.getByRole('button')).toHaveTextContent('first');
});

it('renders 2 buttons initially and shows more after clicking toggle', async () => {
render(<ActionButtons {...fixtures.many} />);
const buttons = screen.getAllByRole('button');
expect(buttons).toHaveLength(2);
expect(screen.getByRole('button', { name: 'first' })).toBeInTheDocument();
const toggleButton = screen.getByRole('button', {
name: 'Menu toggle with action split button'
});
expect(toggleButton).toBeInTheDocument();

expect(screen.queryByText('second')).not.toBeInTheDocument();
expect(screen.queryByText('third')).not.toBeInTheDocument();
await act(async () => fireEvent.click(toggleButton));
expect(screen.getByText('second')).toBeInTheDocument();
expect(screen.getByText('third')).toBeInTheDocument();
const dropdownItems = screen.getAllByRole('menuitem');
expect(dropdownItems).toHaveLength(3);
expect(dropdownItems[2]).toBeDisabled();
});
});

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
.pf-v5-c-menu__content {
ul[role="menu"].action-buttons {
margin-bottom: 0;
}
}
Loading