-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add enum and env var to conditionally render MUI theme #564
Conversation
d15ef8c
to
ee1c21c
Compare
What a great way to show off all your work! |
const [isSwitched, setIsSwitched] = React.useState<boolean>(window.isSwitched ?? false); | ||
|
||
// Listen for changes to `window.isSwitched` | ||
React.useEffect(() => { | ||
const handleSwitchChange = (event: Event) => { | ||
if (event instanceof CustomEvent) { | ||
setIsSwitched(event.detail); // Update state with the new value | ||
} | ||
}; | ||
|
||
window.addEventListener('isSwitchedChanged', handleSwitchChange); | ||
|
||
return () => { | ||
window.removeEventListener('isSwitchedChanged', handleSwitchChange); | ||
}; | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this gets reused a lot I would guess a custom hook like useMUI
would be a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok changes look reaaly great, but overall, we would wanna avoid this global declarations, apologies for not detailing too much in the description the conditional rendering, but the main goal for this is just to turn it on or off at build time, for that:
- Instead of having a global window value, let's just control this with a constant
- That constant could be either a boolean, but I would suggest moving this into an ENUM and then have some utility to control (we only want MUI but it's a good practice to get extensibility)
- We might not want a switcher in the UI, upstream should stick with MUI, this is just for us to be able to contribute both upstream with MUI and downstream with regular PF6 look and feel
- The global const should be an env variable, I'm working right now on a PR to enhance our env variable handling but so far it's gonna be similar on what we have in odh, so you can follow that pattern in the
const.ts
file - Since we don't have proper env support (I'm working on it right now) you can test that in a similar way as we have here just add the env variable you decided to use (maybe STYLE_THEME) to the Kubeflow value (if it's a boolean call it other way) and add that to the dev command, I can tweak it later in my PR
ec4cff7
to
ccd6543
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Griffin-Sullivan @lucferbux for your feedback - updated the PR to reflect your comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, just a couple of nits, everything else working as expected!
clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelVersions/ModelVersionListView.tsx
Outdated
Show resolved
Hide resolved
95eaae9
to
e1f97d0
Compare
Signed-off-by: Jenny <32821331+jenny-s51@users.noreply.github.com>
e1f97d0
to
e3bb6b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ederign, 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:
Approvers can indicate their approval by writing |
/label approved |
@ederign: The label(s) In response to this:
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. |
Description
Adds env var to support toggling MUI theme on / off.
How Has This Been Tested?
Verify that the MUI theme is toggled on / off based on the value of
STYLE_THEME
.Tests N/A, visual updates only.
Merge criteria:
DCO
check)If you have UI changes