Add enum and env var to conditionally render MUI theme #564
Conversation
google-oss-prow
bot
requested review from
alexcreasy,
andreyvelich and
ederign
jenny-s51
force-pushed
the
conditionalThemeFlag
branch
5 times, most recently
from
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.
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.
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.tsfile - 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
jenny-s51
force-pushed
the
conditionalThemeFlag
branch
3 times, most recently
from
ec4cff7 to
ccd6543
Compare
There was a problem hiding this comment.
Thank you @Griffin-Sullivan @lucferbux for your feedback - updated the PR to reflect your comments.
jenny-s51
changed the title
jenny-s51
changed the title
clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelVersions/ModelVersionListView.tsx
Outdated
Show resolved
Hide resolved
jenny-s51
force-pushed
the
conditionalThemeFlag
branch
2 times, most recently
from
95eaae9 to
e1f97d0
Compare
Signed-off-by: Jenny <[email protected]>
jenny-s51
force-pushed
the
conditionalThemeFlag
branch
from
e1f97d0 to
e3bb6b8
Compare
|
[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:
DCOcheck)If you have UI changes