Apply MUI theming to Registered Model List View #432

jenny-s51 · 2024-09-26T13:38:32Z

Description

Applies theming and styles to MR UI for Registered Models List View.

Before:

After:

How Has This Been Tested?

Visual updates only - tested manually in UI.

Merge criteria:

All the commits have been signed-off (To pass the DCO check)

The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work.
Code changes follow the kubeflow contribution guidelines.

If you have UI changes

The developer has added tests or explained why testing cannot be added.
Included any necessary screenshots or gifs if it was a UI change.
Verify that UI/UX changes conform the UX guidelines for Kubeflow.

ederign · 2024-09-26T14:34:46Z

/retest

ederign · 2024-09-26T14:35:23Z

/ok-to-test

ederign · 2024-09-26T14:41:55Z

@tarilabs can you please fire the workflows here?

Griffin-Sullivan · 2024-09-26T15:17:39Z

The filter on the registered models page doesn't change the filter label when selecting Owner. Should say "Find by Owner" here.

Griffin-Sullivan · 2024-09-26T17:30:18Z

@jenny-s51 are we also able to add a little bit of spacing between the filter button and the text in the image above?

clients/ui/frontend/src/style/MUI-theme.scss

Griffin-Sullivan · 2024-09-26T17:41:49Z

clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelRegistry.tsx

@@ -7,6 +7,7 @@ import { filterLiveModels } from '~/app/utils';
 import ModelRegistrySelectorNavigator from './ModelRegistrySelectorNavigator';
 import RegisteredModelListView from './RegisteredModels/RegisteredModelListView';
 import { modelRegistryUrl } from './routeUtils';
+import '~/style/MUI-theme.scss';


I might not understand how all of this works, but are we able to just import this at a higher level so we don't need to import in multiple places?

Good point, removed other imports

@jenny-s51 you can even add it in ModelRegistryRoutes.tsx since it's the entrypoint of MR or even in App.tsx since it's the entrypoint of the project if you want.

Thank you @lucferbux - moved it to ModelRegistryRoutes.tsx

jenny-s51

Thanks for reviewing @Griffin-Sullivan !

The filter on the registered models page doesn't change the filter label when selecting Owner. Should say "Find by Owner" here.

Nice catch, fixed this

@jenny-s51 are we also able to add a little bit of spacing between the filter button and the text in the image above?

Yes, added some padding

lucferbux · 2024-09-27T12:44:19Z

Ok I'm gonna leave a couple of visual elements I would love to discuss here

lucferbux · 2024-09-27T12:44:32Z

Should modals be rectangles too?

lucferbux · 2024-09-27T12:49:21Z

Ok, this is a nit, and I wonder if it's feasible or do you guys agree on that, the central dashboard of kubeflow has this navbar format and the main dashboard is not contained in a square area (similar to patternfly 5):

Whereas our dashboard has the pf6 look, would it be feasible to mimic the look and feel of the kubeflow dashboard for this PoC? If so, should we included in the scope of these changes (or maybe as a follow up?)

lucferbux

Just a couple of nits but overall really great job here! btw, once #431 is merged you can rebase and get the form too.

lucferbux · 2024-09-27T12:50:42Z

clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelRegistry.tsx

@@ -7,6 +7,7 @@ import { filterLiveModels } from '~/app/utils';
 import ModelRegistrySelectorNavigator from './ModelRegistrySelectorNavigator';
 import RegisteredModelListView from './RegisteredModels/RegisteredModelListView';
 import { modelRegistryUrl } from './routeUtils';
+import '~/style/MUI-theme.scss';


@jenny-s51 you can even add it in ModelRegistryRoutes.tsx since it's the entrypoint of MR or even in App.tsx since it's the entrypoint of the project if you want.

Griffin-Sullivan · 2024-09-27T13:03:24Z

@lucferbux I guess we should meet and maybe discuss how far do we want to take the look for the PoC. I don't see a side drawer button for their menu either, and it looks like we would need to mock the user login at the top.

lucferbux · 2024-09-27T13:05:29Z

@lucferbux I guess we should meet and maybe discuss how far do we want to take the look for the PoC. I don't see a side drawer button for their menu either, and it looks like we would need to mock the user login at the top.

Yeah, I'm not talking about a carbon copy though, just at least get the primary colors and the navbar and main content with the same layout, not sure if it's feasible, but then again it's up to a debate.

jenny-s51 · 2024-09-27T14:52:41Z

Should modals be rectangles too?

Yes thank you @lucferbux updated the border radius

jenny-s51

Yeah, I'm not talking about a carbon copy though, just at least get the primary colors and the navbar and main content with the same layout, not sure if it's feasible, but then again it's up to a debate.

Thank you for your awesome, helpful feedback on this @lucferbux !

We should definitely align on the consistency between KF UIs, especially the Kubeflow Central Dashboard. Looking at it in the context of this PR, the changes would involve the UI structure and quite a bit of styling, which is out of scope for this initial theming work of the Registered Model List View.

I wonder if it would make sense to create a new ticket to update the layout and theming of the page and navbar. Let me know your thoughts—I’m happy to open a ticket in JIRA so we can prioritize it for an upcoming sprint. cc @Griffin-Sullivan @ederign

ederign · 2024-09-27T15:59:42Z

@tarilabs can you please fire the GHA on this PR?

Griffin-Sullivan · 2024-09-27T16:34:39Z

Yep those changes are not required for this first PR.

BTW to get past the DCO check can you sign your commit? An easy way to do this when you have already committed is to amend your commit with -s and force push.

git commit --amend -s
git push --force

Signed-off-by: Jenny <32821331+jenny-s51@users.noreply.github.com> adjust padding of label for consistency in focused state fix filter label text, add padding, remove unused imports Signed-off-by: Jenny <32821331+jenny-s51@users.noreply.github.com> update modal border radius, move css import Signed-off-by: Jenny <32821331+jenny-s51@users.noreply.github.com>

jenny-s51 · 2024-09-27T17:24:35Z

Thanks for sharing those commands @Griffin-Sullivan ! Updated and signed the commit.

lucferbux

/lgtm

All changes look incredible, great work here @jenny-s51 🎉

google-oss-prow · 2024-10-01T10:40:50Z

@lucferbux: changing LGTM is restricted to collaborators

In response to this:

/lgtm

All changes look incredible, great work here @jenny-s51 🎉

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

alexcreasy · 2024-10-01T10:56:18Z

This looks amazing! Thanks so much @jenny-s51!!!

alexcreasy · 2024-10-01T10:56:26Z

/lgtm

google-oss-prow · 2024-10-01T10:57:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexcreasy, lucferbux

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:

~~clients/ui/OWNERS~~ [alexcreasy]

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

google-oss-prow bot requested review from alexcreasy, andreyvelich and ederign September 26, 2024 13:38

github-actions bot added the Area/UI label Sep 26, 2024

google-oss-prow bot added the size/XXL label Sep 26, 2024

jenny-s51 force-pushed the applyTheming branch from 66ef76e to 105a115 Compare September 26, 2024 13:40

google-oss-prow bot added the ok-to-test label Sep 26, 2024

Griffin-Sullivan reviewed Sep 26, 2024

View reviewed changes

jenny-s51 force-pushed the applyTheming branch 2 times, most recently from 3468804 to 1541ea4 Compare September 26, 2024 20:26

jenny-s51 commented Sep 26, 2024

View reviewed changes

lucferbux reviewed Sep 27, 2024

View reviewed changes

jenny-s51 force-pushed the applyTheming branch 2 times, most recently from ab59973 to a405ddf Compare September 27, 2024 14:48

jenny-s51 commented Sep 27, 2024

View reviewed changes

jenny-s51 force-pushed the applyTheming branch from a405ddf to 192af54 Compare September 27, 2024 17:22

lucferbux approved these changes Oct 1, 2024

View reviewed changes

google-oss-prow bot assigned alexcreasy Oct 1, 2024

google-oss-prow bot added the lgtm label Oct 1, 2024

alexcreasy approved these changes Oct 1, 2024

View reviewed changes

google-oss-prow bot added the approved label Oct 1, 2024

google-oss-prow bot merged commit 6844a1f into kubeflow:main Oct 1, 2024
3 checks passed

lucferbux mentioned this pull request Oct 1, 2024

Fixes frontend lint errors #444

Closed

7 tasks

jenny-s51 mentioned this pull request Oct 2, 2024

Apply MUI theming to MR UI jenny-s51/model-registry-theming#1

Closed

7 tasks

tarilabs mentioned this pull request Oct 5, 2024

[pull] main from kubeflow:main opendatahub-io/model-registry#130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply MUI theming to Registered Model List View #432

Apply MUI theming to Registered Model List View #432

jenny-s51 commented Sep 26, 2024 •

edited

Loading

ederign commented Sep 26, 2024

ederign commented Sep 26, 2024

ederign commented Sep 26, 2024

Griffin-Sullivan commented Sep 26, 2024 •

edited

Loading

Griffin-Sullivan commented Sep 26, 2024

Griffin-Sullivan Sep 26, 2024

jenny-s51 Sep 26, 2024

lucferbux Sep 27, 2024

jenny-s51 Sep 27, 2024

jenny-s51 left a comment

lucferbux commented Sep 27, 2024

lucferbux commented Sep 27, 2024

lucferbux commented Sep 27, 2024

lucferbux left a comment

lucferbux Sep 27, 2024

Griffin-Sullivan commented Sep 27, 2024

lucferbux commented Sep 27, 2024

jenny-s51 commented Sep 27, 2024 •

edited

Loading

jenny-s51 left a comment •

edited

Loading

ederign commented Sep 27, 2024

Griffin-Sullivan commented Sep 27, 2024

jenny-s51 commented Sep 27, 2024

lucferbux left a comment

google-oss-prow bot commented Oct 1, 2024

alexcreasy commented Oct 1, 2024

alexcreasy commented Oct 1, 2024

google-oss-prow bot commented Oct 1, 2024

Apply MUI theming to Registered Model List View #432

Apply MUI theming to Registered Model List View #432

Conversation

jenny-s51 commented Sep 26, 2024 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

ederign commented Sep 26, 2024

ederign commented Sep 26, 2024

ederign commented Sep 26, 2024

Griffin-Sullivan commented Sep 26, 2024 • edited Loading

Griffin-Sullivan commented Sep 26, 2024

Griffin-Sullivan Sep 26, 2024

Choose a reason for hiding this comment

jenny-s51 Sep 26, 2024

Choose a reason for hiding this comment

lucferbux Sep 27, 2024

Choose a reason for hiding this comment

jenny-s51 Sep 27, 2024

Choose a reason for hiding this comment

jenny-s51 left a comment

Choose a reason for hiding this comment

lucferbux commented Sep 27, 2024

lucferbux commented Sep 27, 2024

lucferbux commented Sep 27, 2024

lucferbux left a comment

Choose a reason for hiding this comment

lucferbux Sep 27, 2024

Choose a reason for hiding this comment

Griffin-Sullivan commented Sep 27, 2024

lucferbux commented Sep 27, 2024

jenny-s51 commented Sep 27, 2024 • edited Loading

jenny-s51 left a comment • edited Loading

Choose a reason for hiding this comment

ederign commented Sep 27, 2024

Griffin-Sullivan commented Sep 27, 2024

jenny-s51 commented Sep 27, 2024

lucferbux left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Oct 1, 2024

alexcreasy commented Oct 1, 2024

alexcreasy commented Oct 1, 2024

google-oss-prow bot commented Oct 1, 2024

jenny-s51 commented Sep 26, 2024 •

edited

Loading

Griffin-Sullivan commented Sep 26, 2024 •

edited

Loading

jenny-s51 commented Sep 27, 2024 •

edited

Loading

jenny-s51 left a comment •

edited

Loading