Fixes #36106: PF5 Refactor - EditorNavbar, EditorOptions #10728

kfamilonidis · 2025-10-17T14:22:12Z

Background

Editor and <EditorNavBar /> upgrade to next version of react requires substitution of Enzyme tests with RTL (React Testing Library) framework.

Acceptance Criteria

Upgrade of components to use PF/5 variants.
- Hosts -> Templates -> Job Templates -> New Job Template (path: /job_templates/new)
- Hosts -> Templates -> Job Templates -> (any or clone) -> Edit (path: /job_templates/xyz/edit)
Removal of Enzyme in all related tests.

Tests

npm run test -- assets/javascripts/react_app/components/Editor assets/javascripts/react_app/components/DiffView

followed by: #10777

webpack/assets/javascripts/react_app/common/IntegrationTestHelper.js

...ets/javascripts/react_app/components/Editor/__tests__/__snapshots__/integration.test.js.snap

webpack/assets/javascripts/react_app/components/Editor/__tests__/integration.test.js

...pts/react_app/components/Editor/components/__tests__/__snapshots__/EditorNavbar.test.js.snap

kfamilonidis

Still needs progress to remove the snaps from the test that are testing components properties, but have noticed that some snaps are different from others; some are keeping store props, and some keeping components props. I would like to ask for some opinion on should we remove both snap types, or should we keep the store props - as the later might be serve as fixtures in the future (maybe transition to other store).

kfamilonidis · 2025-11-10T12:01:58Z

I would like to ask for some opinion on should we remove both snap types, or should we keep the store props - as the later might be serve as fixtures in the future (maybe transition to other store).

The *Actions, *Selectors and *Reducer snapshots are not replaceable with the RTL (React Testing Library) approach. They test logic that falls outside of RTL's context. Similarly, the integration tests snapshots. Importantly, these snapshots are not blocking the major React version upgrade

webpack/assets/javascripts/react_app/common/IntegrationTestHelper.js

Locator for the template nagigation bar (Editor/Changes/Preview) has slightly changed. Related to change theforeman/foreman#10728

ofedoren · 2025-12-16T12:08:05Z

webpack/assets/javascripts/react_app/components/Editor/components/EditorOptions.js

 import PropTypes from 'prop-types';

-import { Button, FormControl } from 'patternfly-react';
+import { FormControl } from 'patternfly-react';


Do we still need this though?

It's used for upload file functionality, but I guess we can use FileUploadField from PF instead.

I feel that we still need this. The tabs UI is handled differently in EditorOptions. upload file renders grouped icons with help menus with some offering form with dropdown; see settings.

FileUploadField will render as Tab. That would be different from the current flow of the user experience. Tabs now are split left-to-right with split in the middle and the ones floating right are grouped together. Changing FileUploadField will be possible breaking the flow of these items that are currently grouped together (show/hide, revert, import, settings).

ofedoren · 2025-12-16T12:39:30Z

webpack/assets/javascripts/react_app/components/Editor/components/EditorOptions.js

          <Tooltip content={__('Import File')} position={TooltipPosition.top}>
            <Button
+              ouiaId="import-file-button"
              disabled={selectedView !== 'input'}


Comment from Copilot is still valid.

yes, this comment is valid. I fixed it, ty. In EditorRadioButton, NavItem isDisabled is breaking the app.

ofedoren · 2025-12-16T12:44:30Z

webpack/assets/javascripts/react_app/components/Editor/components/EditorRadioButton.js

  title,
 }) => (
  <NavItem
+    role="presentation"


What's that for? I don't see it listed as one of the props for NavItem: https://v5-archive.patternfly.org/components/navigation#navitem

is used for linking the test with getByRole instead of using custom css. It's evolution in UI semantics that is included some parts in codebase and I adjusted it the same (because I thought would make more sence). Is used in PF6 as default attribute for NavItem, and here is passed as prop to children.

I'd personally avoid changing internal stuff that we don't control or maybe shouldn't just for the sake of testing. That's what ouia's for. Or we could write a new custom selector getBySomething if current ones are not sufficient enough.

since its an Icon it should have a title and aria-title for accesebility and clarity , we can use that for testing. Also this icon is never really visible, I opened https://issues.redhat.com/browse/SAT-40711 for it

I changed the implementation of NavItem to Tabs. Using the role attribute to link them helped me focus only on the elements that needed changes for the new abstraction. Now, Tabs handle the role attribute internally, and the tests required minimum updates. This ensures the tests are verifying exactly what’s needed.

I don’t think settings belongs as part of the Tabs UI, and it shouldn’t be. However, I agree that the showHide attribute logic is mixed in and a bit obscure throughout the component. That part will need closer attention.

kfamilonidis · 2025-12-17T13:21:06Z

Also seeing how much css overrides we do to make the Nav component behave like Tab component, and with the new pf5 css I noticed also the active tab title are too bright so they will also need an override for the colour, why not just use the tab component?

I have tried to use the NavItem from PF5 with no luck as it seems it doesn't support disable. I agree that Tab could be more appropriate component for this case and I am looking at it. The css overrides could be minimized after this change.

kfamilonidis · 2025-12-18T15:04:01Z

Updated EditorNavBar from NavItems to Tabs. Results look different and more close to PF5 variant without much css overrides. Next is to remove old css completely. SelectHost menu needs a complete new approach. Please see the new look:

Full window width:

Adjusted to smaller size:

With preview select:

MariaAga

Quick notes:

Why is there space between the navbar and the editor?

when clicking on the safemode checkbox nothing happens

Clicking on the right side arrow does not move the navbar to show all the other tools

host select should not increase the size of the navbar

separator line should be in the centre

Copilot

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 20 comments.

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

Copilot · 2025-12-22T12:17:50Z

webpack/assets/javascripts/react_app/components/Editor/components/EditorNavbar.js

+    /* NOTE: raises Each child in a list should have a unique "key" prop.%s%s
+      tabs.push(
+         <div key="preview-spinner" id="preview-spinner">
+           {showPreview && isLoading ? <Spinner size="sm" /> : null}
+         </div>
+       );
+    */


The comment about raising "Each child in a list should have a unique key prop" warning is concerning and suggests there may be an unresolved issue. Since all other items in the tabs array have unique keys, and this code is commented out, it appears the spinner functionality was removed. If the spinner is needed, the issue should be resolved rather than commented out. If it's not needed, the comment block should be removed entirely.

Suggested change

/* NOTE: raises Each child in a list should have a unique "key" prop.%s%s

tabs.push(

<div key="preview-spinner" id="preview-spinner">

{showPreview && isLoading ? <Spinner size="sm" /> : null}

</div>

);

*/

...ck/assets/javascripts/react_app/components/Editor/components/__tests__/EditorOptions.test.js

webpack/assets/javascripts/react_app/components/Editor/__tests__/integration.test.js

...ack/assets/javascripts/react_app/components/Editor/components/__tests__/EditorNavbar.test.js

webpack/assets/javascripts/react_app/components/Editor/components/__tests__/EditorView.test.js

Copilot · 2025-12-22T12:17:54Z

webpack/assets/javascripts/react_app/components/Editor/components/EditorOptions.js

+import { FormControl } from 'patternfly-react';
+import { Button, Icon, Tooltip, TooltipPosition } from '@patternfly/react-core';


FormControl is still imported from the old 'patternfly-react' library (PF3/PF4) instead of '@patternfly/react-core' (PF5). Since this PR is migrating to PF5, this import should be updated to use the PF5 equivalent or a native HTML input element with appropriate styling.

Suggested change

import { FormControl } from 'patternfly-react';

import { Button, Icon, Tooltip, TooltipPosition } from '@patternfly/react-core';

import { Button, Icon, Tooltip, TooltipPosition, FormSelect as FormControl } from '@patternfly/react-core';

webpack/assets/javascripts/react_app/components/Editor/components/EditorHostSelect.js

...k/assets/javascripts/react_app/components/Editor/components/__tests__/EditorSettings.test.js

webpack/assets/javascripts/react_app/components/Editor/components/__tests__/EditorView.test.js

...ssets/javascripts/react_app/components/Editor/components/__tests__/EditorRadioButton.test.js

webpack/assets/javascripts/react_app/components/Editor/__tests__/EditorSelectors.test.js

webpack/assets/javascripts/react_app/components/Editor/EditorConstants.js

webpack/assets/javascripts/react_app/components/Editor/editor.scss

MariaAga · 2025-12-22T12:18:23Z

webpack/assets/javascripts/react_app/components/Editor/components/EditorSettings.js

+              ))}
+            </FormSelect>
+          </FormGroup>
+          <FormGroup


checkboxs and text are still not aligned

MariaAga · 2025-12-22T12:20:08Z

webpack/assets/javascripts/react_app/components/Editor/components/EditorOptions.js

            <Button
+              ouiaId="maximize-editor-button"
              className="editor-button"


fullscreen button is not aligned to the other buttons

thanks. An element ID was missing when styles where applied, now fixed.

about labels and check-boxes centering: did a css override to handle cases that appear inside pf5 forms.

webpack/assets/javascripts/react_app/components/Editor/components/EditorOptions.js

Upgrade NavBar (Alert, Button), Options (RadioButton, View), Settings. Remove enzyme in tests including DiffView, HostSelect, EditorView. Keep actions, and selectors tests.

kfamilonidis · 2025-12-23T13:01:33Z

Quick notes:

Why is there space between the navbar and the editor?

Fixed. Have removed some old css, but still some are present such navbar navbar-form navbar-editor. I try to focus on adding only to one namespace, if needed to override an existing glitch. pls. see .navbar.navbar-editor in editor.scss

when clicking on the safemode checkbox nothing happens

Not sure is tested. Is assumed to be always disabled.

Clicking on the right side arrow does not move the navbar to show all the other tools

Noticed this. It's not a consistent behavior as there are time when it moves, and others not. Need to look at it closer.

host select should not increase the size of the navbar

This is a problem now. The Tab elements are now wrapping the whole Select element, are not floating, and the select element needs to find a way to break out of the normal flow of surounding Tabs.

separator line should be in the centrer

Tried to. Hope it's fixed now.

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

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

kfamilonidis force-pushed the pf5-editor-nav-bar branch 6 times, most recently from 3e7a6c1 to 880cb47 Compare October 21, 2025 12:03

kfamilonidis force-pushed the pf5-editor-nav-bar branch 5 times, most recently from f7cbd8f to 86ff36b Compare October 29, 2025 15:52

kfamilonidis marked this pull request as ready for review October 29, 2025 16:25

kfamilonidis commented Nov 5, 2025

View reviewed changes

webpack/assets/javascripts/react_app/common/IntegrationTestHelper.js Outdated Show resolved Hide resolved

pr-processor bot removed the Not yet reviewed label Nov 5, 2025

kfamilonidis commented Nov 5, 2025

View reviewed changes

...ets/javascripts/react_app/components/Editor/__tests__/__snapshots__/integration.test.js.snap Outdated Show resolved Hide resolved

kfamilonidis commented Nov 5, 2025

View reviewed changes

webpack/assets/javascripts/react_app/components/Editor/__tests__/integration.test.js Outdated Show resolved Hide resolved

kfamilonidis commented Nov 5, 2025

View reviewed changes

...pts/react_app/components/Editor/components/__tests__/__snapshots__/EditorNavbar.test.js.snap Outdated Show resolved Hide resolved

kfamilonidis commented Nov 5, 2025

View reviewed changes

kfamilonidis force-pushed the pf5-editor-nav-bar branch 2 times, most recently from 4474901 to 2fefd25 Compare November 10, 2025 11:51

kfamilonidis force-pushed the pf5-editor-nav-bar branch 6 times, most recently from a7def11 to ebd3141 Compare November 11, 2025 13:53

MariaAga reviewed Nov 11, 2025

View reviewed changes

webpack/assets/javascripts/react_app/common/IntegrationTestHelper.js Outdated Show resolved Hide resolved

pr-processor bot added the Waiting on contributor label Dec 9, 2025

pnovotny added a commit to pnovotny/airgun that referenced this pull request Dec 15, 2025

template editor: update navigation bar locator

a4e89e3

Locator for the template nagigation bar (Editor/Changes/Preview) has slightly changed. Related to change theforeman/foreman#10728

pnovotny mentioned this pull request Dec 15, 2025

template editor: update navigation bar locator SatelliteQE/airgun#2242

Open

ofedoren reviewed Dec 16, 2025

View reviewed changes

kfamilonidis force-pushed the pf5-editor-nav-bar branch from 6989067 to 555686e Compare December 16, 2025 16:21

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

kfamilonidis force-pushed the pf5-editor-nav-bar branch from 555686e to 27874e1 Compare December 18, 2025 14:52

kfamilonidis force-pushed the pf5-editor-nav-bar branch 4 times, most recently from 256ef67 to 7af06d2 Compare December 22, 2025 11:03

kfamilonidis requested review from MariaAga and ofedoren December 22, 2025 11:50

MariaAga requested changes Dec 22, 2025

View reviewed changes

pr-processor bot added the Waiting on contributor label Dec 22, 2025

MariaAga requested a review from Copilot December 22, 2025 12:11

Copilot started reviewing on behalf of MariaAga December 22, 2025 12:11 View session

Copilot AI reviewed Dec 22, 2025

View reviewed changes

MariaAga requested changes Dec 22, 2025

View reviewed changes

MariaAga reviewed Dec 22, 2025

View reviewed changes

webpack/assets/javascripts/react_app/components/Editor/components/EditorOptions.js Outdated Show resolved Hide resolved

Fixes #36106: PF5 Refactor - EditorNavbar, EditorOption

fac1a80

Upgrade NavBar (Alert, Button), Options (RadioButton, View), Settings. Remove enzyme in tests including DiffView, HostSelect, EditorView. Keep actions, and selectors tests.

kfamilonidis force-pushed the pf5-editor-nav-bar branch from 7af06d2 to fac1a80 Compare December 23, 2025 12:28

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

		import { FormControl } from 'patternfly-react';
		import { Button, Icon, Tooltip, TooltipPosition } from '@patternfly/react-core';

Fixes #36106: PF5 Refactor - EditorNavbar, EditorOptions #10728

Are you sure you want to change the base?

Fixes #36106: PF5 Refactor - EditorNavbar, EditorOptions #10728

Conversation

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

Uh oh!

Background

Acceptance Criteria

Tests

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kfamilonidis left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

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!

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 Dec 17, 2025

Uh oh!

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

Uh oh!

Uh oh!

MariaAga left a comment

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!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfamilonidis Dec 23, 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!

Uh oh!

kfamilonidis commented Dec 23, 2025

kfamilonidis commented Oct 17, 2025 •

edited

Loading

kfamilonidis commented Nov 10, 2025 •

edited

Loading

kfamilonidis commented Dec 18, 2025 •

edited

Loading

kfamilonidis Dec 23, 2025 •

edited

Loading