Refs #38822: update pf3 buttons to pf5 #10726

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

Upgrade common/ActionButtons 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

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

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

MariaAga · 2025-11-13T10:15:11Z

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

-      {...firstButton.action}
-      bsSize="small"
+    <Dropdown
+      ouiaId="action-buttons-dropdown"


We probably need to check if theres an ID we can use, so every table row button wont have the same ouiaId

That's correct. Good catch.

This is not been implemented

That's right, correct, I changed the top most button id. The DropDown though it could be relying on any random id, it doesn't rely on any action, I can put random or the first of the collection.

Copilot

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Copilot · 2025-11-13T13:58:04Z

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>


Buttons with href attributes (like the 'second' button in fixtures) may not render correctly. PatternFly 5's Button component requires the component="a" prop to render as an anchor tag. The single button case should be updated:

<Button ouiaId="action-button" size="sm" variant="primary" component={buttons[0].action?.href ? 'a' : undefined} isDisabled={buttons[0].action?.disabled} {...buttons[0].action} >

For DropdownItem, PatternFly 5 uses the to prop for the href value and also may need component="a" depending on the version. Verify the correct API usage for links in dropdown items.

not sure this is a valid point.

Since this is a generic component, it needs to be able to handle onClick but href as well.

<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} to={button.action?.to} component={button.action?.href ? 'a' : undefined} {...button.action} >

And the same for the MenuToggleAction probably

there is no url for href

Couldn't be the test changed to accept getByRole('link')?

yes, but onClick is typically used for safe, controlled actions like API calls, while a standard a tag is a simple link. These can be different use cases where the 'a' tag is a simple link to somewhere, but onClick connected to only to safe actions.

@kfamilonidis How does that effect changing the test to accept getByRole('link') so we can have both urls and onclick?

these are links that perform certain actions, in this use cases (one, many buttons) all buttons are triggering a modal. Considering that these actions are safe, it would be safe to say and is very easy to change all the buttons to a tag and tested by getByRole('link').

But the request is for the split button to support urls, is for future needs not the current ones, and we can have buttons that need to just be an a tag that open a new tab. Or do you suggest to implement opening in a new tab in a different way?
Example of usage with the other type of table action dropdown: https://github.com/theforeman/foreman_remote_execution/blob/master/webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js#L22

kfamilonidis · 2025-11-14T12:49:12Z

All comments have been implemented.

kfamilonidis

To @MariaAga credit, id was found missing.

Upgrade ActionButtons and replace Enzyme tests with RTL. Fix styling an replace bgStyle with equivalent variant props.

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 8396807 to de5a3db Compare November 13, 2025 09:05

MariaAga requested changes Nov 13, 2025

View reviewed changes

pr-processor bot added the Waiting on contributor label Nov 13, 2025

MariaAga requested a review from Copilot November 13, 2025 13:53

Copilot started reviewing on behalf of MariaAga November 13, 2025 13:54 View session

Copilot finished reviewing on behalf of MariaAga November 13, 2025 13:57

Copilot AI reviewed Nov 13, 2025

View reviewed changes

kfamilonidis force-pushed the pf5-buttons branch from de5a3db to bd735de Compare November 14, 2025 12:41

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

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 6 times, most recently from bcc58ac to f46578a Compare November 25, 2025 13:33

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

6fa41ff

Upgrade ActionButtons and replace Enzyme tests with RTL. Fix styling an replace bgStyle with equivalent variant props.

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

Refs #38822: update pf3 buttons to pf5 #10726

Are you sure you want to change the base?

Refs #38822: update pf3 buttons to pf5 #10726

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!

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!

kfamilonidis Nov 18, 2025 • 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

kfamilonidis Nov 14, 2025 • 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!

MariaAga Nov 17, 2025 • 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!

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

Uh oh!

Uh oh!

kfamilonidis left a comment

kfamilonidis commented Oct 16, 2025 •

edited

Loading

MariaAga commented Oct 23, 2025 •

edited

Loading

kfamilonidis Nov 18, 2025 •

edited

Loading

kfamilonidis Nov 14, 2025 •

edited

Loading

MariaAga Nov 17, 2025 •

edited

Loading

kfamilonidis commented Nov 14, 2025 •

edited

Loading