Upgrade odh dashboard to PF5 #1901

jenny-s51 · 2023-10-03T20:08:01Z

Closes #1694

Description

WIP, do not merge

How Has This Been Tested?

Test Impact

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

andrewballantyne · 2023-10-03T21:19:54Z

Great work! Thanks so much for diving in on this @jenny-s51. This will be a fun one to review haha...

/hold

Also let us not merge this into main until we do some testing on the impact side of things. We have a lot of feature branches and this will be interesting to see of what it does to the code, not to mention the timing of a release. Once you get it out of WIP I don't want it to accidentally get approved, so throwing a hold on it as well 🙂

dpanshug

@jenny-s51, There is an issue related to code linting and formatting. To address this, could you please run the following commands:

npm run format
npm run lint:goal

Additionally, it appears that certain components such as Select, Dropdown, and ApplicationLauncher are still imported from PF4. These components have a new implementation and cannot be updated using codemod (source). Are we planning to upgrade these components in this particular issue, or does it require a separate one?

frontend/src/pages/BYONImages/UpdateImageModal.tsx

frontend/src/__tests__/integration/hooks/TestHook.tsx

dpanshug · 2023-10-04T11:44:42Z

Also there is a peer dependency issue with @openshift/dynamic-plugin-sdk-extensions, pls check here

jenny-s51

Hi @dpanshug thanks so much for taking a look at this and for your helpful PR comments. I've addressed a few of them and am still working on resolving some remaining build and UI issues.

As far as updating the deprecated components, that would definitely be the next step, and it would make the most sense to track that work in a follow up issue. Updating the deprecated components may require quite a bit of refactoring, which would be better suited to a review process that is independent of this (already very large) PR. 🙂

frontend/package.json

frontend/src/pages/BYONImages/BYONImagesTable.tsx

dpanshug · 2023-10-09T08:05:04Z

CSS also need to be updated with Patternfly 5, there is a separate codemode for it https://www.patternfly.org/get-started/upgrade#utilize-our-class-name-updater-codemod

andrewballantyne

Self note, we need to log a tech debt to get rid of all the /deprecated imports.

I'll test the UI now, this is just the code review.

backend/src/types.ts

frontend/.cache_16xe4ex

andrewballantyne · 2023-10-10T18:30:09Z

frontend/src/components/OdhAppCard.tsx

+        {
+          <>
+            <BrandImage src={odhApp.spec.img} alt={odhApp.spec.displayName} />
+          </>
+        }


Should this not just be <BrandImage ... />? You're creating a variable block, wrapping a fragment, then rendering a single item 🤔

frontend/src/components/OdhDocCard.tsx

frontend/src/components/OdhExploreCard.tsx

andrewballantyne · 2023-10-10T18:45:36Z

frontend/src/pages/modelServing/screens/metrics/MetricsChart.tsx

@@ -108,7 +108,7 @@ const MetricsChart: React.FC<MetricsChartProps> = ({ title, color, metrics, unit
                </>
              ) : (
                <>
-                  <EmptyStateIcon variant="container" component={Spinner} />
+                  <EmptyStateIcon icon={Spinner} />


Didn't Empty state get a header/footer? Was this missed?

Yes - nice catch @andrewballantyne looks like the codemods didn't pick up this instance

frontend/src/pages/notebookController/screens/server/BrowserTabPreferenceCheckbox.tsx

andrewballantyne · 2023-10-10T18:53:06Z

frontend/src/pages/projects/projectSharing/ProjectSharingNameInput.tsx

+  onChange: (
+    e:
+      | React.MouseEvent<Element, MouseEvent>
+      | React.ChangeEvent<Element>
+      | React.FormEvent<HTMLInputElement>,
+    selection: string,
+  ) => void;


This change seems unnecessary 🤔 Both users only care about selection.

frontend/src/pages/projects/screens/spawner/imageSelector/ImageStreamSelector.tsx

frontend/src/redux/types.ts

andrewballantyne

Some UI issues from my testing....

Each are inline to allow for comments... except for this one:

I'm not sure how this one broke... internal CSS for PF & our overrides due to bad tab formats... is my guess, since you didn't modify the file in question (frontend/src/concepts/pipelines/content/pipelinesDetails/pipeline/PipelineDetails.tsx)

The sidebar in the tab breaks scroll area for the topology. We lose the bottom left icons (if you scroll the sidebar all the way to the bottom, they reappear).

Before:

After:

andrewballantyne · 2023-10-10T19:17:59Z

frontend/src/pages/notebookController/screens/server/EnvironmentVariablesField.tsx

+                  onUpdateVariable({ name: variable.name, type: variable.type, value: newValue })
+                }
+              />
+            </InputGroupItem>
            {variable.type === 'password' ? (


Before:

After:

I am unable to reproduce both the topology issue (above) and this environment variables issue in my local instance of the shared cluster. Would be great if the team could help investigate these issues cc @andrewballantyne

andrewballantyne · 2023-10-10T19:20:46Z

frontend/src/concepts/k8s/NameDescriptionField.tsx

+            {!disableK8sName && (
+              <FormHelperText>
+                <HelperText>
+                  <HelperTextItem icon={<ExclamationCircleIcon />} variant={'error'}>


Before:

After:

andrewballantyne · 2023-10-10T19:22:48Z

...end/src/pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeTokenInput.tsx

+                variant={token.error ? 'error' : 'default'}
+              >
+                {token.error ??
+                  'Enter the service account name for which the token will be generated'}


Before:

After:

andrewballantyne · 2023-10-10T19:25:05Z

.../pages/modelServing/screens/projects/InferenceServiceModal/DataConnectionFolderPathField.tsx

+              >
+                {validated === 'error'
+                  ? 'The path must not point to a root folder.'
+                  : 'Enter a path to a model or folder. This path cannot point to a root folder.'}


Before:

After:

andrewballantyne · 2023-10-10T19:30:03Z

frontend/src/pages/clusterSettings/PVCSizeSettings.tsx

+                }}
+              />
+            </InputGroupItem>
+            <InputGroupText>GiB</InputGroupText>


And in frontend/src/pages/clusterSettings/CullerSettings.tsx -- same issue, I caught them both in the same screenshot and I'm too lazy to retake or to break up the screenshots 🙂

Before:

After:

andrewballantyne

Replies to your latest code push

andrewballantyne · 2023-10-10T19:36:02Z

frontend/src/components/OdhDocCard.tsx

+          actions: (
+            <>
+              <FavoriteButton isFavorite={favorite} onClick={() => updateFavorite(!favorite)} />
+            </>


This appears to be a missed point of my previous review... mind removing the fragment here too?

andrewballantyne · 2023-10-10T19:37:52Z

frontend/src/components/OdhExploreCard.tsx

+        {...(!dashboardConfig.spec.dashboardConfig.disableISVBadges && {
+          actions: {
+            actions: (
+              <>


Fragments are not needed when dealing with 1 child... they help when you have two or more siblings and you don't have a comment parent (usually only needed when passing as a "single prop" or "returning as a single value").

andrewballantyne · 2023-10-10T19:42:13Z

frontend/src/redux/types.ts

@@ -11,7 +11,6 @@ export enum Actions {
  FORCE_COMPONENTS_UPDATE = 'FORCE_COMPONENTS_UPDATE',
 }

-export type AppNotificationStatus = 'success' | 'danger' | 'warning' | 'info' | 'default';


Umm... maybe just undo all of this... not sure what you were changing 🤔 I think these values are fine.

#1901 (comment)

Ah I think you're addressing PF errors, and you modified this to fit those. These are our notification values. These are used in frontend/src/app/AppNotificationDrawer.tsx

Where are the variants:

variant?: 'custom' | 'success' | 'danger' | 'warning' | 'info';

Can we just make this a TS reference of that?

Updated to reference the PF AlertVariant enum let me know if this is what you had in mind @andrewballantyne

dpanshug · 2023-10-11T05:05:20Z

need to log a tech debt to get rid of all the /deprecated imports.

@andrewballantyne I already logged a feature issue #1931

DaoDaoNoCode

The hover (see image 1) and selected (see image 2) card styles seem broken. Looks like PF changed the props interface for `CardHeader`, we might want to look into that to make sure the style works and keeps the functionality at the same time.

jenny-s51 · 2023-10-11T20:57:27Z

The hover (see image 1) and selected (see image 2) card styles seem broken. Looks like PF changed the props interface for CardHeader, we might want to look into that to make sure the style works and keeps the functionality at the same time.

Thank you @DaoDaoNoCode nice catch!

I've updated OdhExploreCard to use the new PF5 recommendation for the "clickable and selectable" pattern, which can now be seen in the "Enable" view.

Still investigating why this update does not apply to the "Jupyter" card ... do you have any ideas how we can fix this?

DaoDaoNoCode · 2023-10-11T23:37:49Z

@jenny-s51 Yes, it might related to the props in CardHeader component, you may need to use selectableActions instead of actions there, however, I am not sure how it could be compatible with the current implementation, you might need some test to see if anything is broken.

changes to card component, revert openshift-dynamic-sdk bump fix dependency conflicts, remove comments, lint upgrade hardcocded class references to v5 classnames run codemods after rebase run linting commands fix lint errors in CI workflow comment out references to PluginLoader format sdkInitialize update pluginstore initialization update classnames and change Title to EmptyStateHeader remove cache files update to latest release versions format remove unused title import add EmptyStateActions import PR feedback from Andrew PR feedback from Andrew and Juntao apply patch format remove eslint no console remove unused imports fix missing link in Jupyter card

andrewballantyne · 2023-10-17T18:59:35Z

/unhold
/retest

Reworking this branch to merge into a feature branch for a brief time. This won't go to incubation yet, we will look to get this safely into main, hopefully this sprint -- might be next sprint.

There is still a few little items to tie up in the style department from our custom styles.

Thanks again for the hard work here @jenny-s51 🎉

openshift-ci · 2023-10-17T19:00:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [andrewballantyne]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jenny-s51 added the do-not-merge/work-in-progress This PR is in WIP state label Oct 3, 2023

openshift-ci bot requested review from manaswinidas and ppadti October 3, 2023 20:08

openshift-merge-robot added the needs-rebase PR needs to be rebased label Oct 3, 2023

jenny-s51 force-pushed the pf5_upgrade branch from 3a72ddf to f0b9cf3 Compare October 3, 2023 20:15

openshift-merge-robot removed the needs-rebase PR needs to be rebased label Oct 3, 2023

openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Oct 3, 2023

dpanshug requested changes Oct 4, 2023

View reviewed changes

frontend/src/pages/BYONImages/UpdateImageModal.tsx Outdated Show resolved Hide resolved

frontend/src/__tests__/integration/hooks/TestHook.tsx Outdated Show resolved Hide resolved

openshift-ci bot assigned dpanshug Oct 4, 2023

openshift-merge-robot added the needs-rebase PR needs to be rebased label Oct 4, 2023

jenny-s51 commented Oct 4, 2023

View reviewed changes

dpanshug reviewed Oct 5, 2023

View reviewed changes

frontend/package.json Show resolved Hide resolved

jenny-s51 force-pushed the pf5_upgrade branch 2 times, most recently from 62bba0e to a9d5b03 Compare October 6, 2023 15:52

openshift-merge-robot removed the needs-rebase PR needs to be rebased label Oct 6, 2023

dpanshug reviewed Oct 9, 2023

View reviewed changes

frontend/src/pages/BYONImages/BYONImagesTable.tsx Outdated Show resolved Hide resolved

jenny-s51 force-pushed the pf5_upgrade branch from 892523a to b1c1c27 Compare October 10, 2023 18:10

andrewballantyne requested changes Oct 10, 2023

View reviewed changes

openshift-ci bot assigned andrewballantyne Oct 10, 2023

andrewballantyne requested changes Oct 10, 2023

View reviewed changes

DaoDaoNoCode reviewed Oct 11, 2023

View reviewed changes

jenny-s51 force-pushed the pf5_upgrade branch from 53ea6d7 to e8a797a Compare October 11, 2023 20:46

jenny-s51 force-pushed the pf5_upgrade branch from 2a0c4a9 to d544ab8 Compare October 11, 2023 21:01

jenny-s51 force-pushed the pf5_upgrade branch from 8ec4b51 to 5620a68 Compare October 17, 2023 17:32

andrewballantyne changed the base branch from main to f/pf5-upgrade October 17, 2023 18:58

andrewballantyne changed the title ~~[WIP] Upgrade odh dashboard to PF5~~ Upgrade odh dashboard to PF5 Oct 17, 2023

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Oct 17, 2023

openshift-ci bot removed the do-not-merge/hold This PR is hold for some reason label Oct 17, 2023

andrewballantyne approved these changes Oct 17, 2023

View reviewed changes

openshift-ci bot added the lgtm label Oct 17, 2023

openshift-ci bot added the approved label Oct 17, 2023

openshift-ci bot merged commit 33c36b7 into opendatahub-io:f/pf5-upgrade Oct 17, 2023
6 checks passed

andrewballantyne mentioned this pull request Oct 11, 2023

[Feature Request]: Upgrade to Patternfly v5 #1694

Closed

jenny-s51 mentioned this pull request Oct 20, 2023

PF5 upgrade: styling updates #2000

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade odh dashboard to PF5 #1901

Upgrade odh dashboard to PF5 #1901

jenny-s51 commented Oct 3, 2023

andrewballantyne commented Oct 3, 2023

dpanshug left a comment

dpanshug commented Oct 4, 2023

jenny-s51 left a comment •

edited

Loading

dpanshug commented Oct 9, 2023

andrewballantyne left a comment

andrewballantyne Oct 10, 2023

andrewballantyne Oct 10, 2023

jenny-s51 Oct 10, 2023

andrewballantyne Oct 10, 2023

andrewballantyne left a comment

andrewballantyne Oct 10, 2023

jenny-s51 Oct 11, 2023 •

edited

Loading

andrewballantyne Oct 10, 2023

andrewballantyne Oct 10, 2023

andrewballantyne Oct 10, 2023

andrewballantyne Oct 10, 2023

andrewballantyne left a comment

andrewballantyne Oct 10, 2023

andrewballantyne Oct 10, 2023

andrewballantyne Oct 10, 2023

andrewballantyne Oct 10, 2023

jenny-s51 Oct 11, 2023

dpanshug commented Oct 11, 2023 •

edited

Loading

DaoDaoNoCode left a comment

jenny-s51 commented Oct 11, 2023 •

edited

Loading

DaoDaoNoCode commented Oct 11, 2023

andrewballantyne commented Oct 17, 2023

openshift-ci bot commented Oct 17, 2023

Upgrade odh dashboard to PF5 #1901

Upgrade odh dashboard to PF5 #1901

Conversation

jenny-s51 commented Oct 3, 2023

Description

How Has This Been Tested?

Test Impact

Request review criteria:

andrewballantyne commented Oct 3, 2023

dpanshug left a comment

Choose a reason for hiding this comment

dpanshug commented Oct 4, 2023

jenny-s51 left a comment • edited Loading

Choose a reason for hiding this comment

dpanshug commented Oct 9, 2023

andrewballantyne left a 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

andrewballantyne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenny-s51 Oct 11, 2023 • edited Loading

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

andrewballantyne left a 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

dpanshug commented Oct 11, 2023 • edited Loading

DaoDaoNoCode left a comment

Choose a reason for hiding this comment

jenny-s51 commented Oct 11, 2023 • edited Loading

DaoDaoNoCode commented Oct 11, 2023

andrewballantyne commented Oct 17, 2023

openshift-ci bot commented Oct 17, 2023

jenny-s51 left a comment •

edited

Loading

jenny-s51 Oct 11, 2023 •

edited

Loading

dpanshug commented Oct 11, 2023 •

edited

Loading

jenny-s51 commented Oct 11, 2023 •

edited

Loading