webui: port deprecated Wizard component to new PF5 implementation by adamkankovsky · Pull Request #5179 · rhinstaller/anaconda

adamkankovsky · 2023-09-15T13:54:25Z

In the end, the changes were a bit more extensive than I expected, but I tried to keep as much clarity and as much functionality as possible.

rvykydal · 2023-09-18T13:44:08Z

-        if (prevStep.prevId === "installation-review" && newStep.id !== "installation-progress") {
+        if (prevStep.prevId === "installation-review" && currentStep.id !== "installation-progress") {
            setIsInProgress(true);
            resetPartitioning()


Seems that resetPartitioning is not called when moving from installation-review back. Which corresponds with failing TestBasic.testSidebarNavigation.

Ah, you need to replace prevId with id (also at the other place).
Apart from that, you just seem to need some updates (rebase) for pixel tests to make them pass.

Ah one more thing probably related also to one of the failing pixel tests: TestStorageMountPoints-testBasic-mount-point-mapping-table. When you go back (either by Back or in sidebar steps, for example to Installation Method) the next steps (for example Review and Install) are no more disabled (which they used to be).
It is probably caused by canJumpTo being removed from WizardStep in PF5

rvykydal · 2023-09-19T08:14:36Z

+                id: s.id,
                stepNavItemProps: { id: s.id },
+                ...(s.isExpandable && { isExpandable: true }),
                canJumpTo: canJumpToStep(s.id, currentStepId),


canJumpTo seems to be removed in PF5, I guess we'll need to use isDisabled instead and the logic used in canJumpToStep will need to be modified a bit.

rvykydal

Need some update at least due to missing isPrev and canJumpTo in PF5. More details inline.

KKoukiou · 2023-09-28T06:49:10Z

    const [isFormValid, setIsFormValid] = useState(false);
    const [stepNotification, setStepNotification] = useState();
    const [isInProgress, setIsInProgress] = useState(false);
+    const [isInstallationProgress, setInstallationProgress] = useState(false);


There is no need (it's weird) for this to be state variable. It should just be constant.

const inInstallationProgressStep = path[0] === "installation-progress";

KKoukiou · 2023-09-28T06:49:53Z

            data: {
                deviceData: storageData.devices,
                diskSelection: storageData.diskSelection,
-                requests: storageData.partitioning ? storageData.partitioning.requests : null,


Thanks for shorting alphabetically the properties here and in the imports, but please do that in seperate commit if possible for code readability purposes.

KKoukiou · 2023-09-28T06:50:18Z

                diskSelection: storageData.diskSelection,
-                requests: storageData.partitioning ? storageData.partitioning.requests : null,
                language,
-                osRelease


osRelease here was removed? Is that on the purpose?

Maybe it's a special commit again, but osRelease is sold by default to every component in componentProps and it was so even in drive, it's unnecessary to have it there twice

KKoukiou · 2023-09-28T06:56:29Z

-    const steps = createSteps(stepsOrder);
+    const steps = createSteps(stepsOrder, componentProps);

-    const goToStep = (newStep, prevStep) => {


Do you mind keeping the variable naming how is was before, just to cause less diff in the commit? Let's limit the commit to the required changes. Any improvements can come if you like in follow up, and be discussed separately.

Yes, it looks quite redundant here, but otherwise usually when I changed variable names, it was in connection with similar implementations in patternfly demos, but I don't know if this was the case.

KKoukiou · 2023-09-28T06:57:24Z

                    setNextWaitsConfirmation(false);
-                    onNext();
+                    setInstallationProgress(true);
+                    window.location.hash = "#/installation-progress";


Please use cockpit.location.go instead of writing to the window.location.hash object.

KKoukiou · 2023-10-10T07:17:49Z

@garrett this PR has a design question for you.
In anaconda Wizard generally we can only move forward one step at a time. We allow the user to go back multiple steps at once by clicking the NAV menu, but then if they want to go forward again they need to use 'Next' button and go one by one.

This in Patternfly is defined as 'progressive steps'.
https://www.patternfly.org/components/wizard/#progressive-steps

The Patternfly 5 when used with progressive steps, hides all the future steps (which makes sense IMO) whilst Patternfly 4 just disables these, so they can't be accessed from the NAV. The hiding makes sense IMO as the future steps are dependent on the selections inside the previous steps. (See Encryption is available only if mount point assignment is not selected)

So, are you ok with introducing this UI change with the PF5 porting, and hide the future steps as seen in the PF5 demo above?

garrett · 2023-10-10T08:17:09Z

Being able to figure out where you are in a process and estimate how much longer you have left is one of core functions in a Wizard.

According to Nielsen Norman Group (well respected team who have written many of the fundamental books in the realm of software design): https://www.nngroup.com/articles/wizards/ — section "Recommendations for Designing Usable Wizards", subsection 2:

"Communicate a clear mental model of the process by displaying a list or a diagram of the steps involved and highlighting the current step.

Because the process is presented one step at a time, there is danger that users will miss the context, get confused, or not realize how long the progression will be. It best to set the right expectations and explain to the use what the process looks like and how many steps are involved.

Especially with complicated processes, a clear labeling of each of the steps as well as a clear indicator of the current step can help keep users oriented."

I would strongly suggest we only using progressive steps in sub-steps, not major steps. Keep the major steps consistent, so there's a goal to work toward.

If there's a previous page that nullifies a future page, then drop the future page, but keep other future pages that still apply. That is: Dynamically adjust future pages, if needed. But don't hide all future pages.

Our Wizard is following the progressive steps model, as we can only move forward one step at time. The new PF5 implementation hides the steps that are in the future when the progressive model is chosen, so the only way forward is the 'Next' button. Previously the only difference was that the steps were visible just disabled. Co-authored by: Adam Kaňkovský <akankovs@redhat.com>

github-actions Bot added webui f40 labels Sep 15, 2023

adamkankovsky changed the title ~~Port deprecated components in Patternfly 5 to new implementation - Wizard~~ Webui: Port deprecated components in Patternfly 5 to new implementation - Wizard Sep 15, 2023

rvykydal reviewed Sep 18, 2023

View reviewed changes

rvykydal requested review from rvykydal and removed request for rvykydal September 19, 2023 06:38

rvykydal reviewed Sep 19, 2023

View reviewed changes

rvykydal requested changes Sep 19, 2023

View reviewed changes

adamkankovsky force-pushed the webui-porting-deprecated-wizard branch 9 times, most recently from f3e32cc to 6feda93 Compare September 21, 2023 11:59

github-advanced-security AI found potential problems Sep 21, 2023

View reviewed changes

Comment thread ui/webui/src/components/AnacondaWizard.jsx Fixed

adamkankovsky force-pushed the webui-porting-deprecated-wizard branch 5 times, most recently from 1b1413f to be91a7a Compare September 25, 2023 13:42

adamkankovsky requested a review from KKoukiou September 25, 2023 14:10

adamkankovsky force-pushed the webui-porting-deprecated-wizard branch 2 times, most recently from 4dbdbc9 to 930c3f3 Compare September 26, 2023 12:00

rhinstaller deleted a comment from pep8speaks Sep 26, 2023

KKoukiou reviewed Sep 28, 2023

View reviewed changes

KKoukiou force-pushed the webui-porting-deprecated-wizard branch from 930c3f3 to cba64f1 Compare September 29, 2023 08:54

adamkankovsky force-pushed the webui-porting-deprecated-wizard branch from cba64f1 to 7f4ac3c Compare September 29, 2023 09:46

adamkankovsky force-pushed the webui-porting-deprecated-wizard branch 2 times, most recently from 3af3776 to 20d14c3 Compare October 10, 2023 13:10

adamkankovsky force-pushed the webui-porting-deprecated-wizard branch from 20d14c3 to 8167044 Compare October 10, 2023 14:50

KKoukiou changed the title ~~Webui: Port deprecated components in Patternfly 5 to new implementation - Wizard~~ webui: port deprecated Wizard component to new PF5 implementation Oct 10, 2023

adamkankovsky mentioned this pull request Oct 11, 2023

Bug - wizard - onStepChange return hidden substep patternfly/patternfly-react#9736

Closed

adamkankovsky added the blocked Don't merge this pull request! label Oct 11, 2023

KKoukiou closed this Nov 20, 2023

Uh oh!

Conversation

adamkankovsky commented Sep 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvykydal Sep 18, 2023 • 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!

rvykydal Sep 19, 2023 • 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!

rvykydal Sep 19, 2023 • 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!

rvykydal left a comment

Choose a reason for hiding this comment

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!

KKoukiou commented Oct 10, 2023

Uh oh!

garrett commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

rvykydal Sep 18, 2023 •

edited

Loading

rvykydal Sep 19, 2023 •

edited

Loading

rvykydal Sep 19, 2023 •

edited

Loading

garrett commented Oct 10, 2023 •

edited

Loading