Migrate deprecated Wizard component to PF5 and fix Wizard's focus issue #1428

sgratch · 2025-01-06T15:47:27Z

References:
https://issues.redhat.com/browse/MTV-1051
https://issues.redhat.com/browse/MTV-1863

Migrate the deprecated Wizard component to PF5 - see https://v5-archive.patternfly.org/components/wizard/react-deprecated
When moving between steps in plan's wizard, set the initial focus to the top of the step's page.
When clicking the Cancel button within the wizard, instead of always navigating to history.goBack(),
the following happens now:
If the wizard is triggered by the provider - vms tab => cancel will navigate back to the provider - vms tab
If the wizard is triggered by the plans list page => cancel will navigate back to the plans list page
If the wizard is triggered by entering the wizard url directly => cancel will navigate back to the plans list page.

Screencasts

Before

Screencast.from.2025-01-06.18-33-35.webm

After

Screencast.from.2025-01-06.18-38-54.webm

metalice · 2025-01-07T06:18:06Z

packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePage.tsx

+          shouldFocusContent
+          title={title}
+          startIndex={startAtStep}
+          onClose={() => history.goBack()}


this can be a bit problematic sometimes, for example if someone enter directly to wizard URL, onclose will send out of the app - consider navigate to a list page

I fixed it as follows:
If the wizard is triggered by the provider - vms tab => cancel will navigate back to the provider - vms tab
If the wizard is triggered by the plans list page => cancel will navigate back to the plans list page
If the wizard is triggered by entering the wizard url directly => cancel will navigate back to the plans list page.

This is another bug fix, so added that to main comment as well.

metalice · 2025-01-07T06:22:22Z

packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePage.tsx

+                Object.values(state?.validation || []).some(
+                  (validation) => validation === 'error',
+                ) ||
+                state?.validation?.planName === 'default',


seems like state.validation can be object or array?

also consider extracting it to a isDsiabled func

validation is an object property within the CreateVmMigrationPageState interface, this huge state object is very complicated to track and debug as is so I prefer to leave it and not extract it to a local env.

But I extract it to separate funcs to make it more readable.

codecov-commenter · 2025-01-09T21:22:17Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.20%. Comparing base (13484d0) to head (c2baf0d).
Report is 188 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1428      +/-   ##
==========================================
- Coverage   36.81%   36.20%   -0.62%     
==========================================
  Files         158      159       +1     
  Lines        2548     2580      +32     
  Branches      599      616      +17     
==========================================
- Hits          938      934       -4     
- Misses       1428     1451      +23     
- Partials      182      195      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePage.tsx

metalice · 2025-01-16T17:00:10Z

packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePage.tsx

+          title={title}
+          startIndex={startAtStep}
+          onClose={() =>
+            window.location.replace(createPlanFromPlansList ? plansListURL : `${providerURL}/vms`)


Why touch the direct window location and not use navigate from the useNavigate hook?

The navigate hook is supported by React Router v6: https://reactrouter.com/en/v6.3.0/upgrading/v5#use-usenavigate-instead-of-usehistory.
The kubevirt plugin and console code are still using [email protected] with react-router-dom-v5-compat and no usage found for the useNavigate.
I tried using react-router-dom-v5-compat here as wel,l just to be aligned, but it doesn't help of course and I'm not sure we can upgrade to 6 before the console does.
Are you aware of a plugin using the useNavigate hook?

so u have history() form use* hook of react-router lib,
BTW this is also a tech debt as the console will stop supporting router v5

metalice · 2025-01-16T17:00:21Z

packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePage.tsx

+    (validation) => validation === 'error',
+  );
+
+  const planNameValidationDefault = state?.validation?.planName === 'default';


consider useMemo

The state is fetched using the useFetchEffects and I can't really calculate all the dependencies, so there is no real advantage in using useMemo here.

what do u mean by I cant calculate all dependences?
there is only 1 dep which is state , u have many other values in this file that will cause to rerender, that arent related to the state

The whole State changed after almost every change so it's not effective (checked). I added useMemo and changed the dep to use the specific state?.validation?.planName here...

metalice · 2025-01-16T17:00:47Z

packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePage.tsx

+  const anyValidationError = Object.values(state?.validation || []).some(
+    (validation) => validation === 'error',
+  );


consider useMemo and extract to a util file

Extracted to a util file.
Regarding useMemo - again since it's fetched from the big migration state, see my comment above.

As mentioned in previous comment, state is changed a lot and as a result, the validations, so there is no point in adding useMemo here.

other parts in this component are causing rerenders that aren't related to the state - so you will run this even if the state isn't changing.

the useMemo should have only state as a dep

@metalice I'll change it as you request,
but have a general best practices question:
React's memorization tools make the code less readable and a bit more complicated and therefore if there is no real influence on performance that can be visible or we know in advance that might happen in scaled envs, why using it always?
What are the cases that we can avoid using memo hooks?

packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePage.tsx

References: https://issues.redhat.com/browse/MTV-1051 https://issues.redhat.com/browse/MTV-1863 1. Migrate the deprecated Wizard component to PF5 - see https://v5-archive.patternfly.org/components/wizard/react-deprecated 2. When moving between steps in plan wizard, set the initial focus to the top of the step's page. Signed-off-by: Sharon Gratch <[email protected]>

sonarqubecloud · 2025-01-22T12:38:14Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

sgratch added bug Categorizes issue or PR as related to a bug. enhancement Categorizes issue or PR as related to a new feature. labels Jan 6, 2025

sgratch added this to the 2.8.0 milestone Jan 6, 2025

sgratch requested a review from yaacov January 6, 2025 16:22

metalice reviewed Jan 7, 2025

View reviewed changes

sgratch force-pushed the Migrate-deprecated-wizard-to-pf5-and-fix-focus-bug branch 2 times, most recently from 9bfc03d to c2c63c6 Compare January 9, 2025 20:55

sgratch requested a review from metalice January 9, 2025 21:04

metalice reviewed Jan 16, 2025

View reviewed changes

packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePage.tsx Outdated Show resolved Hide resolved

metalice reviewed Jan 16, 2025

View reviewed changes

packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePage.tsx Show resolved Hide resolved

metalice reviewed Jan 16, 2025

View reviewed changes

packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePage.tsx Outdated Show resolved Hide resolved

metalice reviewed Jan 16, 2025

View reviewed changes

packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePage.tsx Outdated Show resolved Hide resolved

sgratch force-pushed the Migrate-deprecated-wizard-to-pf5-and-fix-focus-bug branch from c2c63c6 to c77613c Compare January 17, 2025 09:53

sgratch requested a review from metalice January 17, 2025 10:04

sgratch force-pushed the Migrate-deprecated-wizard-to-pf5-and-fix-focus-bug branch from c77613c to 9f8cb80 Compare January 21, 2025 18:10

sgratch force-pushed the Migrate-deprecated-wizard-to-pf5-and-fix-focus-bug branch from 9f8cb80 to c2baf0d Compare January 22, 2025 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate deprecated Wizard component to PF5 and fix Wizard's focus issue #1428

Migrate deprecated Wizard component to PF5 and fix Wizard's focus issue #1428

sgratch commented Jan 6, 2025 •

edited

Loading

metalice Jan 7, 2025

sgratch Jan 9, 2025

metalice Jan 7, 2025

sgratch Jan 9, 2025

codecov-commenter commented Jan 9, 2025 •

edited

Loading

metalice Jan 16, 2025

sgratch Jan 17, 2025

metalice Jan 20, 2025

metalice Jan 16, 2025

sgratch Jan 17, 2025

metalice Jan 20, 2025

sgratch Jan 21, 2025

metalice Jan 16, 2025

sgratch Jan 17, 2025

sgratch Jan 21, 2025 •

edited

Loading

metalice Jan 22, 2025

metalice Jan 22, 2025

sgratch Jan 22, 2025 •

edited

Loading

sonarqubecloud bot commented Jan 22, 2025

Migrate deprecated Wizard component to PF5 and fix Wizard's focus issue #1428

Are you sure you want to change the base?

Migrate deprecated Wizard component to PF5 and fix Wizard's focus issue #1428

Conversation

sgratch commented Jan 6, 2025 • edited Loading

Screencasts

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 9, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgratch Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgratch Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 22, 2025

Quality Gate passed

sgratch commented Jan 6, 2025 •

edited

Loading

codecov-commenter commented Jan 9, 2025 •

edited

Loading

sgratch Jan 21, 2025 •

edited

Loading

sgratch Jan 22, 2025 •

edited

Loading