Skip to content

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

Open
kfamilonidis wants to merge 1 commit intotheforeman:developfrom
kfamilonidis:pf5-editor-nav-bar
Open

Fixes #36106: PF5 Refactor - EditorNavbar, EditorOptions#10728
kfamilonidis wants to merge 1 commit intotheforeman:developfrom
kfamilonidis:pf5-editor-nav-bar

Conversation

@kfamilonidis
Copy link
Contributor

@kfamilonidis kfamilonidis commented Oct 17, 2025

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

@github-actions github-actions bot added the UI label Oct 17, 2025
@kfamilonidis kfamilonidis force-pushed the pf5-editor-nav-bar branch 6 times, most recently from 3e7a6c1 to 880cb47 Compare October 21, 2025 12:03
@kfamilonidis kfamilonidis force-pushed the pf5-editor-nav-bar branch 5 times, most recently from f7cbd8f to 86ff36b Compare October 29, 2025 15:52
@kfamilonidis kfamilonidis marked this pull request as ready for review October 29, 2025 16:25
Copy link
Contributor Author

@kfamilonidis kfamilonidis left a comment

Choose a reason for hiding this comment

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

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 kfamilonidis force-pushed the pf5-editor-nav-bar branch 2 times, most recently from 4474901 to 2fefd25 Compare November 10, 2025 11:51
@kfamilonidis
Copy link
Contributor Author

kfamilonidis commented Nov 10, 2025

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

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

Progress update:

  • when clicking on the safemode checkbox nothing happens
    You can enable safemode in administer -> settings -> provisioning -> Safemode rendering = no

Tested it. Enabling and clicking it doesn't trigger the event. It needs to be triggered by the select menu change. When clicking the event is not bound to refresh with selected value. Following the existing flow, when clicked and select it correctly pass the parameter. /job_templates/171-Check%20Update%20-%20Script%20Default%20clone/preview?force_safemode=true

Screenshot From 2026-01-06 13-14-24
  • Changing from Editor to Changes changes the height of the navbar as it adds extra space

This is fixed. It was caused by margin applied. I tried to center all elements. pls. see:
Screenshot From 2026-01-06 12-37-30

  • host select should not increase the size of the navbar

This is fixed. Position of item changed to static. There is still a glitch with viewport when the side-bars appear in lower screen resolution which need a closer look (fix is going to apply to any container).

Screenshot From 2026-01-06 12-02-37 Screenshot From 2026-01-06 12-02-50

Pending:

  • Clicking on the right side arrow does not move the navbar to show all the other tools
  • NEW: alignment of select menu when the side-bars appear in lower screen resolution

@MariaAga
Copy link
Member

MariaAga commented Jan 6, 2026

Tested it. Enabling and clicking it doesn't trigger the event. It needs to be triggered by the select menu change. When clicking the event is not bound to refresh with selected value. Following the existing flow, when clicked and select it correctly pass the parameter. /job_templates/171-Check%20Update%20-%20Script%20Default%20clone/preview?force_safemode=true

so is it on the pending changes as well? as for this version I still see the checkbox and clicking on it does not enable the safemode as it should

@kfamilonidis
Copy link
Contributor Author

Tested it. Enabling and clicking it doesn't trigger the event. It needs to be triggered by the select menu change. When clicking the event is not bound to refresh with selected value. Following the existing flow, when clicked and select it correctly pass the parameter. /job_templates/171-Check%20Update%20-%20Script%20Default%20clone/preview?force_safemode=true

so is it on the pending changes as well? as for this version I still see the checkbox and clicking on it does not enable the safemode as it should

It should be, provided that the onClick was there before, or can I can add it to the list:

  • NEW: Clicking on the checkbox triggers refresh event on selected host item

@MariaAga
Copy link
Member

MariaAga commented Jan 6, 2026

To clarify, here are screen captures
before this PR: clicking on the checkbox, adds and removes the checked mark

Screencast.From.2026-01-06.12-59-14.mp4

After: clicking on the checkbox does nothing (it does select the tab text)

Screencast.From.2026-01-06.12-58-19.mp4

@kfamilonidis
Copy link
Contributor Author

To clarify, here are screen captures before this PR: clicking on the checkbox, adds and removes the checked mark

Sure, thanks. The issue is fixed by z-index value, pls confirm. Probably zIndex needs to be tackled as this affects also the select menu area in low resolution mode.

@MariaAga
Copy link
Member

MariaAga commented Jan 6, 2026

The z-inedx change for the checkbox causes over-flow into the maximized editor:
image

@kfamilonidis
Copy link
Contributor Author

The z-inedx change for the checkbox causes over-flow into the maximized editor:

To clarify:
Similar z-index problem applies for the select menu:
Screenshot From 2026-01-06 19-08-11
Screenshot From 2026-01-06 19-07-38
Screenshot From 2026-01-06 19-07-29
Screenshot From 2026-01-06 19-10-29

I propose to handle the zIndex layers holistically to target a smooth result in all places that zIndex is affecting the visual elements (toolbar, modal).

@MariaAga
Copy link
Member

MariaAga commented Jan 6, 2026

I propose to handle the zIndex layers holistically to target a smooth result in all places that zIndex is affecting the visual elements (toolbar, modal).

Can you describe how that will work in practise?

@kfamilonidis
Copy link
Contributor Author

I propose to handle the zIndex layers holistically to target a smooth result in all places that zIndex is affecting the visual elements (toolbar, modal).

Can you describe how that will work in practise?

I think giving effort to adjust this specific element (select menu) with custom css (zindex overrides) that affects checkbox (block) is hard to justify. This PR #10777 is to incorporate a new design in select menu that can behave differently. Do you propose any alternative ?

Screenshot From 2026-01-07 09-46-14 Screenshot From 2026-01-07 09-37-22

@MariaAga
Copy link
Member

MariaAga commented Jan 7, 2026

I am talking about fixing the safemode checkbox.
2 options to fix the current state:

  1. replace webpack/assets/javascripts/react_app/components/Editor/components/EditorSafemodeCheckbox.js with a PF5 checkbox, that way there is no z-index issues as PF elements work together
  2. Change the z-index from 1000, which is higher than the modal, to a number that is higher than the nav bar ul element z-index, but lower than the modal z index (500). For example setting the safemode z-index to 400 ensures that the checkbox can be checked, and is not visible on top of the modal.

@kfamilonidis kfamilonidis force-pushed the pf5-editor-nav-bar branch 2 times, most recently from ad8e8f8 to 0e04bb0 Compare January 7, 2026 11:31
@kfamilonidis
Copy link
Contributor Author

thanks @MariaAga. The first option seemed more straightforward.

Screenshot From 2026-01-07 13-24-07 Screenshot From 2026-01-07 13-29-23

Tested it. Works as expected. Other remain are:

Upgrade NavBar (Alert, Button), Options (RadioButton, View), Settings.
Remove enzyme in tests including DiffView, HostSelect, EditorView. Keep
actions, and selectors tests.
pnovotny added a commit to pnovotny/airgun that referenced this pull request Jan 14, 2026
Locator for the template nagigation bar (Editor/Changes/Preview) has slightly changed.

Related to change theforeman/foreman#10728
@pnovotny
Copy link

I've retested the changes and everything looks good to me, i.e.:

  • navigating through the tabs Editor/Changes/Preview(*)
  • right side icons Undo, Import, Settings, Fullscreen editor work correctly, no issues found there

I can confirm the few minor leftovers mentioned by @kfamilonidis in his comment #10728 (comment)
I would also add one small UX thing to the list:

  • the host menu on the Preview tab is expanded only when clicking the small caret icon on the right, but it should expand when clicking on the whole dropdown button - a.k.a. clicking on the host name:
editor-host-select-expand

@MariaAga
Copy link
Member

@pnovotny

the host menu on the Preview tab is expanded only when clicking the small caret icon on the right, but it should expand when clicking on the whole dropdown button - a.k.a. clicking on the host name:

Is this a regression? if not then it can be addressed here instead: #10777

@pnovotny
Copy link

@MariaAga yes, I forgot to mention that it is a regression. In the old editor, the menu expands when clicking wherever on the host select.
But I wouldn't consider this as a blocker for merging this PR. Since it is related only to the host select, it can be IMO handled as part of #10777 anyway.

@adamlazik1
Copy link
Contributor

adamlazik1 commented Feb 4, 2026

Hello @MariaAga and @pnovotny

Regarding what still needs to be done in this PR, here are my suggestions:

  • the host menu on the Preview tab is expanded only when clicking the small caret icon on the right, but it should expand when clicking on the whole dropdown button - a.k.a. clicking on the host name:
  • Select Menu overflow on lower resolution screen (which can be worked with Fixes #38916 - Update EditorHostSelect to pf5 #10777)

I propose these to be handled in #10777.

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

I failed to reproduce this. @pnovotny could I get reproducing steps for this issue?

@MariaAga
Copy link
Member

MariaAga commented Feb 4, 2026

@adamlazik1

I failed to reproduce this. @pnovotny could I get reproducing steps for this issue?

you can use the dev console to make the screen smaller if you dont see the arrows

@adamlazik1
Copy link
Contributor

@adamlazik1

I failed to reproduce this. @pnovotny could I get reproducing steps for this issue?

you can use the dev console to make the screen smaller if you dont see the arrows

I see the arrows but to me they work as expected.

@pnovotny
Copy link

pnovotny commented Feb 4, 2026

I see the arrows but to me they work as expected.

@adamlazik1 Ok, let me retry it on the latest snap build (163). I've reproduced it on snap 155 which is now quite old.

@pnovotny
Copy link

pnovotny commented Feb 5, 2026

@adamlazik1 I've tried this PR (foreman-3.18.0-0.20260112104949807834.pr10728.8933.gf5ff9739e.el9) on top of snap build 163.
The horizontal arrows on a narrow screen are working fine in most cases.
I found just one caveat whet it doesn't on the Preview tab. There is some play regarding to the Safemode checkbox, where the right arrow doesn't move the navbar.
Here's the screen recording:
screenrecord-2026-02-05_13.58.27.webm

@adamlazik1
Copy link
Contributor

@adamlazik1 I've tried this PR (foreman-3.18.0-0.20260112104949807834.pr10728.8933.gf5ff9739e.el9) on top of snap build 163. The horizontal arrows on a narrow screen are working fine in most cases. I found just one caveat whet it doesn't on the Preview tab. There is some play regarding to the Safemode checkbox, where the right arrow doesn't move the navbar. Here's the screen recording: screenrecord-2026-02-05_13.58.27.webm

Thanks, I will investigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants