feat(frontend): Create theme provider by jenny-s51 · Pull Request #936 · kubeflow/hub

jenny-s51 · 2025-04-01T19:53:24Z

Model registry UI uses env variables to switch themes which requires a completely separate build in order to use a different theme. When consuming model registry UI through module federation, it should be possible to assign the theme at runtime.

It will use an env variable to assign the default theme and it should also be possible to switch themes at runtime.

Description

Creates a new react context and wraps the entire app from within bootstrap.tsx.
Changes all instances of isMUITheme() function call to a isMUITheme variable and assign the variable from the context.

How Has This Been Tested?

On line 17 of const.ts , change Theme.MUI to Theme.Default and verify UI updates theme without a new build.

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.
For first time contributors: Please reach out to the Reviewers to ensure all tests are being run, ensuring the label ok-to-test has been added to the PR.

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.

christianvogt · 2025-04-02T21:09:27Z

/lgtm

tested mui and default themes

google-oss-prow · 2025-04-02T21:09:31Z

@christianvogt: changing LGTM is restricted to collaborators

Details

In response to this:

/lgtm

tested mui and default themes

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.

lucferbux

I wanna have a conversation about this, need to know what's the main purpose.

lucferbux · 2025-04-04T12:27:34Z

@@ -0,0 +1,11 @@
+import * as React from 'react';
+
+type ThemeContextProps = {


This is just a nit, but in the dashboard we tend to get both Context and ContextProvider in the same file, we usually call the file <specific-name>Context and inside we have both things. can we follow that pattern here?

Nice catch @lucferbux - renamed the ThemeProvider.tsx to ThemeContext.tsx which applies the pattern you mention here: https://github.com/kubeflow/model-registry/pull/936/files#diff-3d6c136c679e4743b6813190737e2a47ec4a71e6283d06c9da4546fa7c8a522f

@jenny-s51 Lucas is suggesting you move both the provider and context into the same file (also include the hook). Looks like there's still two different files: MUIThemeContext and ThemeContext

lucferbux · 2025-04-04T12:28:29Z

@@ -0,0 +1,35 @@
+import * as React from 'react';


Another nit, but I'll fix that in my PR, this should go to the shared library since it's gonna be used everywhere, I'm refactoring that a little bit so we can keep it here for this PR.

Sounds good, thanks @lucferbux - opened opendatahub-io/mod-arch-library#4

lucferbux · 2025-04-04T12:47:34Z

Ok, deleted one of the comments, I didn't understand the main goal, now it's clear, if we can address the nit of the ...ContextProvider convention @jenny-s51 would be ok for me. cc @christianvogt

christianvogt · 2025-04-04T13:13:08Z

+};
+
+export const ThemeContext = React.createContext({
+  isMUITheme: true,


@jenny-s51 I think this should be set to false to match the default component library being used. And the default style.

Or maybe this can read the env variable?

Thanks @christianvogt! Updated to false

Signed-off-by: Jenny <32821331+jenny-s51@users.noreply.github.com> fix UI loading bug Signed-off-by: Jenny <32821331+jenny-s51@users.noreply.github.com> rename ThemeProvider, remove conditional rendering for upstream Signed-off-by: Jenny <32821331+jenny-s51@users.noreply.github.com> remove unused imports Signed-off-by: Jenny <32821331+jenny-s51@users.noreply.github.com> delete ThemeContext.ts and move logic to ThemeContext.tsx Signed-off-by: Jenny <32821331+jenny-s51@users.noreply.github.com> reapply conditionals Signed-off-by: Jenny <32821331+jenny-s51@users.noreply.github.com> revert to isMUITheme

lucferbux

/lgtm

lucferbux · 2025-04-07T17:00:46Z

        <EmptyModelRegistryState
          testid="empty-model-registries-state"
-          title={isMUITheme() ? 'Deploy a model registry' : 'Request access to model registries'}
+          title="Deploy a model registry"


we should still have both text here, in kubeflow we deploy a model registry but midstream we reqeust access to it.

we either remove it everywhere (see line 70) or we maintain the conditional rendering.

lucferbux · 2025-04-07T17:02:46Z

-        ) : (
-          <Divider />
-        )
+        <TitleWithIcon title="Model Registry" objectType={ProjectObjectType.registeredModels} />


Same here, in muiTheme we don't want TitleWithIcon we cant the title with the divider, that's the look with mui, with default is the this way. And muitheme shouldn't have a description.

lucferbux · 2025-04-07T17:15:14Z

-          <Divider />
-        )
-      }
+      title="Model Registry"


Here we are. goin with title rather than title with icon, let's just make it more consisten, we need to move the conversation to agree on a solution.

lucferbux · 2025-04-07T17:15:40Z

-            popoverPosition={PopoverPosition.left}
-          />
-        )}
+        <KubeflowDocs


I can agree that the administration section should not live upstream.

lucferbux · 2025-04-07T17:16:23Z

-                      data-testid="model-versions-table-search"
-                    />
-                  )}
+                  <FormFieldset


If we are allowing to set the flag upstream, this will introduce breaks if a user decide to set THEME=default ?

google-oss-prow · 2025-04-08T07:37:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lucferbux

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~clients/ui/OWNERS~~ [lucferbux]

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

jenny-s51 marked this pull request as draft April 1, 2025 19:53

google-oss-prow Bot added the do-not-merge/work-in-progress label Apr 1, 2025

google-oss-prow Bot requested review from Al-Pragliola and andreyvelich April 1, 2025 19:53

github-actions Bot added the Area/UI label Apr 1, 2025

google-oss-prow Bot added the size/L label Apr 1, 2025

jenny-s51 marked this pull request as ready for review April 2, 2025 19:53

google-oss-prow Bot removed the do-not-merge/work-in-progress label Apr 2, 2025

jenny-s51 force-pushed the create-theme-provider branch from 7c18faf to cdca5ef Compare April 2, 2025 19:53

lucferbux suggested changes Apr 4, 2025

View reviewed changes

christianvogt reviewed Apr 4, 2025

View reviewed changes

jenny-s51 mentioned this pull request Apr 4, 2025

Add Context for Theme Provider opendatahub-io/mod-arch-library#4

Closed

google-oss-prow Bot added size/XL and removed size/L labels Apr 7, 2025

jenny-s51 force-pushed the create-theme-provider branch 4 times, most recently from ab524d3 to 7914113 Compare April 7, 2025 17:12

google-oss-prow Bot added size/L and removed size/XL labels Apr 7, 2025

jenny-s51 force-pushed the create-theme-provider branch from b04a139 to 69c5db7 Compare April 7, 2025 17:33

jenny-s51 force-pushed the create-theme-provider branch from 4c367b1 to 95421b8 Compare April 7, 2025 17:42

lucferbux approved these changes Apr 8, 2025

View reviewed changes

google-oss-prow Bot assigned lucferbux Apr 8, 2025

google-oss-prow Bot added the lgtm label Apr 8, 2025

google-oss-prow Bot added the approved label Apr 8, 2025

google-oss-prow Bot merged commit c4d4613 into kubeflow:main Apr 8, 2025
18 checks passed

Al-Pragliola mentioned this pull request Apr 18, 2025

periodic sync upstream KF to midstream ODH opendatahub-io/model-registry#192

Merged

		@@ -0,0 +1,11 @@
		import * as React from 'react';

		type ThemeContextProps = {

Conversation

jenny-s51 commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How Has This Been Tested?

Merge criteria:

Uh oh!

christianvogt commented Apr 2, 2025

Uh oh!

google-oss-prow Bot commented Apr 2, 2025

Uh oh!

lucferbux left a comment

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!

lucferbux commented Apr 4, 2025

Uh oh!

christianvogt Apr 4, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

lucferbux left a comment

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!

google-oss-prow Bot commented Apr 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jenny-s51 commented Apr 1, 2025 •

edited

Loading

christianvogt Apr 4, 2025 •

edited

Loading