Refs #38822: update pf3 buttons to pf5 by kfamilonidis · Pull Request #10726 · theforeman/foreman

kfamilonidis · 2025-10-16T10:27:49Z

Upgrade common/ActionButtons, layout/ExportButton, and replace Enzyme tests with React Testing Library. Fix styling and replace bgStyle with equivalent variant props.

Acceptance Criteria:

ActionButtons: actions should appear in dropdown list when more than 1.
- /webhooks -> index table.
- /foreman_tasks/tasks -> index table

followed by #10766

refs: theforeman/foreman-tasks#787

RefreshButton:
ExportButton

Copilot

Pull Request Overview

This PR upgrades button components from PatternFly 3 to PatternFly 5 and replaces Enzyme-based tests with React Testing Library tests. The changes ensure consistent button styling and improve test quality across the application.

Migrated ActionButtons, DeleteButton, DiffModal, ForemanModalFooter, ExportButton, SubmitOrCancel, and other button components from PF3 to PF5
Replaced deprecated bsStyle props with variant props and updated button styling properties
Converted all component tests from Enzyme snapshot testing to React Testing Library with proper assertions

Reviewed Changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
ActionButtons.js	Replaced SplitButton with Dropdown and MenuToggle components for PF5 compatibility
ExportButton.js	Updated Button import and props, added ouiaId for testing
PersonalAccessToken.js	Migrated button styling from className to variant/size props
DeleteButton.js	Updated Button props and added ouiaId
Actions.js	Updated form action buttons with PF5 props and ouiaIds
DiffModal.js	Updated close button styling
ForemanModalFooter.js	Updated close button with PF5 variant
SubmitBtn.js/CancelBtn.js	Updated button variants and added ouiaIds
Test files	Converted from Enzyme snapshots to RTL with proper assertions and interactions

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

...sets/javascripts/react_app/components/ForemanModal/subcomponents/SubmitOrCancel/CancelBtn.js

webpack/assets/javascripts/react_app/components/common/ActionButtons/ActionButtons.js

Lukshio

Hi, thanks,
I only found some things to change.

webpack/assets/javascripts/react_app/components/common/ActionButtons/ActionButtons.js

webpack/assets/javascripts/react_app/components/common/ActionButtons/ActionButtons.fixtures.js

MariaAga

This looks like a duplication of webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/ActionButtons.js
Since common/actionButtons is used in plugins, I think the best way would be to move TableIndexPage/ActionButtons import to import from common, and combine the two components

kfamilonidis · 2025-10-22T14:46:02Z

This looks like a duplication of webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/ActionButtons.js Since common/actionButtons is used in plugins, I think the best way would be to move TableIndexPage/ActionButtons import to import from common, and combine the two components

It's not a duplicate, the WebhooksIndexPage uses ActionButtons as table/row, while JobsInvocationDetail page (that uses PF4/TableIndexPage/ActionButtons, through TableIndex) as toolbar. There are few more occurrences of TableIndexPage but haven't found other Button dependencies that relate to ActionButtons functionality. Also, the PF4/TableIndexPage/ActionButtons.js are using upgraded buttons already.

foreman_tasks/tasks also uses this change from import common/ActionButtons/ActionButtons in ForemanTasks/Components/common/ActionButtons/ActionButton.js

MariaAga · 2025-10-23T08:28:18Z

@MariSvirik Is it ok that in PF5 we will have 2 types of action buttons in the table? (kebab and button) or should we choose one, and which?
(She is on PTO so we will probably get an answer next week)

kfamilonidis · 2025-11-11T15:17:39Z

I think that this PR with reduced scope of only one item in the list is safe to progress.

MariaAga · 2025-11-12T11:08:57Z

I talked to Maria S, who talked to other designers as well, and its ok to have the 2 different table buttons, so I will review this task as it is

kfamilonidis

To @MariaAga credit, id was found missing.

kfamilonidis · 2025-12-05T14:43:09Z

uuidV1 was added to ouiaId to give buttons a unique identifier. This provides a foundation that future uses can adjust or extend as necessary.

ofedoren · 2025-12-16T13:57:33Z

webpack/assets/javascripts/react_app/components/common/ActionButtons/ActionButtons.js

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.

thanks @ofedoren. The uuidV1 was not necessary.

ofedoren · 2025-12-16T14:00:53Z

webpack/assets/javascripts/react_app/components/common/ActionButtons/ActionButtons.js

+                key="split-action"
+                aria-label={firstButton.title}
+              >
+                {firstButton.title}


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

that was correct. It has been resolved (also added isDisabled)

ofedoren · 2025-12-16T14:02:30Z

webpack/assets/javascripts/react_app/components/common/ActionButtons/ActionButtons.js

+            ouiaId={`${button.action?.id}-dropdown-item-id`}
+            key={`${button.action?.id}-dropdown-item-key`}
+            title={button.title}
+            onClick={button.action?.onClick}


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?

Correct, menu item href isn't supported, and it's not used anywhere else except 2 places which are handled during testing.

ofedoren · 2025-12-16T14:04:04Z

webpack/assets/javascripts/react_app/components/common/ActionButtons/ActionButtons.js

+      isOpen={isOpen}
+      onOpenChange={openState => setIsOpen(openState)}
+      toggle={toggleRef => (
+        <MenuToggle


should MenuToggle support isDisabled as well?

added it. But, in case the buttons are only one, this case is handled differently.

ofedoren · 2025-12-16T14:05:36Z

BTW, could you please change the commit prefix to be Fixes instead of Refs, so at least this PR is treated as a "main" fix.

ekohl · 2025-12-22T17:44:58Z

BTW, could you please change the commit prefix to be Fixes instead of Refs, so at least this PR is treated as a "main" fix.

Is it? After this there are still PR there many files that use PF3 buttons:

$ rg 'Button.+patternfly-react'
webpack/assets/javascripts/react_app/routes/common/PageLayout/components/ExportButton/ExportButton.js
2:import { Button } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/users/PersonalAccessTokens/PersonalAccessTokensList/PersonalAccessToken.js
3:import { Button } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/index.js
3:import { Alert, Button } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/index.js
2:import { Button } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js
2:import { Button } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/common/table/components/DeleteButton.js
3:import { Button } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/common/forms/Actions.js
3:import { Button } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/common/RedirectCancelButton/RedirectCancelButton.js
2:import { Button } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/ConfigReports/DiffModal/DiffModal.js
2:import { Modal, Button } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/Editor/components/EditorSettings.js
3:import { Dropdown, MenuItem, Button } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/ForemanModal/subcomponents/ForemanModalFooter.js
3:import { Modal, Button } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/Editor/components/EditorOptions.js
5:import { Button, FormControl } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/Editor/components/EditorNavbar.js
3:import { Nav, Spinner, Alert, Button } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/ForemanModal/subcomponents/__tests__/ForemanModalFooter.test.js
3:import { Button, Modal } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/ForemanModal/subcomponents/SubmitOrCancel/SubmitBtn.js
2:import { Button } from 'patternfly-react';

webpack/assets/javascripts/react_app/components/ForemanModal/subcomponents/SubmitOrCancel/CancelBtn.js
2:import { Button } from 'patternfly-react';

ofedoren · 2026-01-02T12:56:09Z

Is it? After this there are still PR there many files that use PF3 buttons:

True.

It was just a suggestion so we don't end up with refs only PRs/commits for the whole button update ticket. Personally, I don't mind it to be splitted into bunch of smaller PRs so we get them faster, but on the other hand it feels weird to merge refs before fixes.

Thinking about it more I might have done a mistake merging one of them :/ We could just treat the main ticket as a tracker and create for each PR a dedicated small ticket. That would be good for progress tracking as well as we won't have refs hell 🤷

kfamilonidis

Extended the scope to include ExportButton (layout), and DeleteButton (table) as they seem to be close related and can be tested together. Have to rename the previous PR's to refrect this change. Not sure how to properly handle the uuidV1 yet.

kfamilonidis · 2026-01-05T15:35:39Z

webpack/assets/javascripts/react_app/components/common/ActionButtons/ActionButtons.js

+            ouiaId={`${button.action?.id}-dropdown-item-id`}
+            key={`${button.action?.id}-dropdown-item-key`}
+            title={button.title}
+            onClick={button.action?.onClick}


Correct, menu item href isn't supported, and it's not used anywhere else except 2 places which are handled during testing.

kfamilonidis · 2026-01-05T15:36:48Z

webpack/assets/javascripts/react_app/components/common/ActionButtons/ActionButtons.js

+                key="split-action"
+                aria-label={firstButton.title}
+              >
+                {firstButton.title}


that was correct. It has been resolved (also added isDisabled)

kfamilonidis · 2026-01-05T16:21:05Z

webpack/assets/javascripts/react_app/components/common/ActionButtons/ActionButtons.js

+      <Button
+        ouiaId="action-button"
+        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"
+      isOpen={isOpen}
+      onOpenChange={openState => setIsOpen(openState)}
+      toggle={toggleRef => (
+        <MenuToggle
+          ref={toggleRef}
+          onClick={onToggleClick}
+          isExpanded={isOpen}
+          splitButtonOptions={{
+            variant: 'action',
+            items: [
+              <MenuToggleAction
+                id={`split-button-action-${firstButton.action?.id}}-toggle-button`}
+                key="split-action"
+                aria-label={firstButton.title}
+                onClick={firstButton.action?.onClick}
+              >
+                {firstButton.title}
+              </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}
+            isDisabled={button.action?.disabled}
+          >
+            {button.title}
+          </DropdownItem>


It seems to be more closer to this https://issues.redhat.com/browse/SAT-32118 tasks.

kfamilonidis · 2026-01-05T16:22:58Z

webpack/assets/javascripts/react_app/components/common/ActionButtons/ActionButtons.js

+      isOpen={isOpen}
+      onOpenChange={openState => setIsOpen(openState)}
+      toggle={toggleRef => (
+        <MenuToggle


added it. But, in case the buttons are only one, this case is handled differently.

MariaAga · 2026-01-08T11:12:10Z

...ssets/javascripts/react_app/routes/common/PageLayout/components/ExportButton/ExportButton.js

 import { translate as __ } from '../../../../../common/I18n';
 import { exportURL } from '../../../../../common/urlHelpers';

 const ExportButton = ({ url, title, text }) => (


Since this is only used in foreman tasks, I think this should be deprecated, and all the buttons in the all tasks page toolbar, next to the search, should be updated together

I don't have strong option on this. It is true that there is a connection there already exists, and it depends on what core component are linked. I think it should stay (there is also the potential ActionButtons that goes in pair in this list), so there are 2 already (and 1 only for Refresh Data).

Ok, then please open a PR in foreman-tasks so this and that PR can be merged at the same time.

I added it, thanks.

MariaAga · 2026-01-08T11:18:01Z

webpack/assets/javascripts/react_app/components/common/ActionButtons/ActionButtons.js

+      isOpen={isOpen}
+      onOpenChange={openState => setIsOpen(openState)}
+      toggle={toggleRef => (
+        <MenuToggle


since the button has variant=primary the menutoggle should have it as well for consistency

I changed it.

MariaAga · 2026-01-08T11:21:00Z

webpack/assets/javascripts/react_app/components/common/table/components/DeleteButton.js

+import { Button } from '@patternfly/react-core';
 import { translate as __ } from '../../../../common/I18n';

 const DeleteButton = ({ active, onClick }) =>


I couldnt find any use of this component, can it be deprecated?

no, its use is inside deleteActionCell.js. It seems that all these buttons emerge to the common/Table. There is a pattern there.

Where is deleteActionCellFormatter used? how did you test this change?

there are places the common/Table exists. This change didn't came from a specific use case. I assume should be in many places common/Table is usaged (webhooks, tasks). I can update the AC and investigate conditions that can trigger this button.

Please always test the changes you make in a PR and validate that the changes in the code are present in the application. This is relevant to all PRs.
Our guidelines also request "Suggest prerequisites for testing and testing scenarios following example above." which means we request a clear explanation on how to test the changes, reviewers and testers rely on the PR description to find the conditions that trigger this button to they can test it. The PR can't be approved without testing it.

there are tests that test the functionality of the Button in JS. The canDelete property that is passed to it exists only in WebhooksPageSelectors, and WebhooksTable (different project - imports the same formatter). This functionality is tested in there foreman-webhooks. foreman-tasks don't use this function (probably, can't delete a task) but the function is there (included in tests).

Please share a screenshot with instructions how to see it, where the delete button is shown. Testing the feature in a PR also means opening the application and testing its visibility and functionality.

I looked at AC and there are no such scenarios. I can remove those tests from there and add a test to ActionButtons to test the active/disable ability instead. This guarantees that the first button will always be safe to press (deleteButton), while the rest buttons functionality can be considered safe (if the first button is not disabled). However, this is a significant change, and it's difficult to predict the full impact of it.

ekohl · 2026-01-09T15:52:01Z

webpack/assets/javascripts/react_app/components/common/table/components/DeleteButton.js

ouia stands for Open UI Automation and ouiaId has 1 purpose: create automation in UI testing. That implies they must be stable and using UUIDs in them defeats their whole purpose. See https://www.patternfly.org/developer-resources/open-ui-automation/ for more.

The specifications are clear. These would succeed as valid ouia-Ids for testing:

OUIA:Component – For components and component frameworks
OUIA:Page – For page identification
OUIA:PageSafe – For interaction safety

It also states:

ARIA is designed as an accessibility specification, and it doesn't account for aspects that are crucial for automation such as a component type. The id attribute might be reserved by some front-end code, and there might be restrictions on changing it.

This implies that uniqueness validation may not be necessary in cases where the id is already covered by another identifier. Though, other framework-specific validations could still apply.

Omitting uniqueness validation would be safe, provided the framework imposes no restrictions. Uniqueness is guaranteed by the backend specification (there is no table without ID validation).

Open UI Automation also serves another purpose: enabling assisted guidance for accessibility. In this context, IDs are relatively minor - no user relying on assisted guidance would require that level of detail.

In automation, we often use locators like '//div[contains(@data-ouia-component-id, "OUIA-Generated-Modal-large-")]'. Stable ouia IDs are the best from our perspective but as you can see, if it's something unstable appended to something stable and unique, we can still work with it. That said, if there is no reason to add UUID (that is, there is only one component of that type in the page), I wouldn't add the UUID.

We also use ARIA labels in locators sometimes when OUIA is not available but we strongly prefer OUIA.

Upgrade ActionButtons, DeleteButton, ExportButton. Replace Enzyme tests with RTL. Fix styling an replace bgStyle with equivalent variant props.

kfamilonidis · 2026-01-13T16:26:19Z

Have put my efforts to do the final adjustments for new review.

pr-processor bot added the Not yet reviewed label Oct 16, 2025

github-actions bot added the UI label Oct 16, 2025

kfamilonidis force-pushed the pf5-buttons branch 2 times, most recently from 12b46df to 5d9ee95 Compare October 16, 2025 12:13

MariaAga requested a review from Copilot October 16, 2025 12:31

MariaAga added the breaks-robottelo label Oct 16, 2025

Copilot AI reviewed Oct 16, 2025

View reviewed changes

pr-processor bot removed the Not yet reviewed label Oct 16, 2025

kfamilonidis force-pushed the pf5-buttons branch 5 times, most recently from c23a763 to 46df6d4 Compare October 20, 2025 11:52

Lukshio suggested changes Oct 20, 2025

View reviewed changes

pr-processor bot added Waiting on contributor Needs re-review labels Oct 20, 2025

MariaAga requested changes Oct 21, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Oct 21, 2025

kfamilonidis force-pushed the pf5-buttons branch from b254470 to a0d88ee Compare October 21, 2025 12:50

pr-processor bot added Needs re-review and removed Waiting on contributor labels Oct 21, 2025

kfamilonidis force-pushed the pf5-buttons branch from a0d88ee to 730069a Compare October 21, 2025 13:09

pr-processor bot removed the Needs re-review label Oct 21, 2025

kfamilonidis requested a review from MariaAga October 22, 2025 16:15

kfamilonidis changed the title ~~Fixes #38822: update pf3 buttons to pf5~~ Refs #38822: update pf3 buttons to pf5 Nov 12, 2025

kfamilonidis force-pushed the pf5-buttons branch from 730069a to 8396807 Compare November 12, 2025 13:48

kfamilonidis force-pushed the pf5-buttons branch from bd735de to 7053ac0 Compare November 14, 2025 12:52

kfamilonidis commented Nov 18, 2025

View reviewed changes

kfamilonidis force-pushed the pf5-buttons branch 7 times, most recently from f46578a to 6fa41ff Compare November 25, 2025 14:28

kfamilonidis requested review from MariaAga and kmalyjur December 8, 2025 14:15

ofedoren reviewed Dec 16, 2025

View reviewed changes

kfamilonidis force-pushed the pf5-buttons branch 3 times, most recently from 3d80c0e to 03b9654 Compare January 5, 2026 15:57

kfamilonidis commented Jan 5, 2026

View reviewed changes

MariaAga reviewed Jan 8, 2026

View reviewed changes

kfamilonidis mentioned this pull request Jan 8, 2026

Fixes #38984: Upgrade PF3 to PF5 buttons theforeman/foreman-tasks#787

Open

ekohl reviewed Jan 9, 2026

View reviewed changes

kfamilonidis force-pushed the pf5-buttons branch from 03b9654 to 8566b96 Compare January 13, 2026 13:29

Refs #38822: update pf3 buttons to pf5 - common/ActionButtons, Table

f431a55

Upgrade ActionButtons, DeleteButton, ExportButton. Replace Enzyme tests with RTL. Fix styling an replace bgStyle with equivalent variant props.

kfamilonidis force-pushed the pf5-buttons branch from 8566b96 to f431a55 Compare January 13, 2026 15:28

kfamilonidis requested a review from MariaAga January 13, 2026 16:26

Conversation

kfamilonidis commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lukshio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MariaAga left a comment

Choose a reason for hiding this comment

Uh oh!

kfamilonidis commented Oct 22, 2025

Uh oh!

MariaAga commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfamilonidis commented Nov 11, 2025

Uh oh!

MariaAga commented Nov 12, 2025

Uh oh!

kfamilonidis left a comment

Choose a reason for hiding this comment

Uh oh!

kfamilonidis commented Dec 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ofedoren commented Dec 16, 2025

Uh oh!

ekohl commented Dec 22, 2025

Uh oh!

ofedoren commented Jan 2, 2026

Uh oh!

kfamilonidis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kfamilonidis commented Oct 16, 2025 •

edited

Loading

MariaAga commented Oct 23, 2025 •

edited

Loading

kfamilonidis left a comment •

edited

Loading

MariaAga Jan 8, 2026 •

edited

Loading

MariaAga Jan 9, 2026 •

edited

Loading

kfamilonidis Jan 13, 2026 •

edited

Loading

kfamilonidis Jan 13, 2026 •

edited

Loading

lhellebr Jan 14, 2026 •

edited

Loading