Add flag to conditionally render MUI theme #564
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-oss-prow
bot
requested review from
alexcreasy,
andreyvelich and
ederign
jenny-s51
force-pushed
the
conditionalThemeFlag
branch
4 times, most recently
from
4b81c4a
to
acccbe1
Compare
Signed-off-by: Jenny <[email protected]> format Signed-off-by: Jenny <[email protected]> fix tests Signed-off-by: Jenny <[email protected]> remove unused imports Signed-off-by: Jenny <[email protected]> fix plain button border radius and editable label padding
jenny-s51
force-pushed
the
conditionalThemeFlag
branch
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
2 times, most recently
from
a14f287
to
b8fef6b
Compare
Signed-off-by: Jenny <[email protected]> format fix build errors Signed-off-by: Jenny <[email protected]> fix import order and linting Signed-off-by: Jenny <[email protected]> Add process.env.STYLE_THEME
jenny-s51
force-pushed
the
conditionalThemeFlag
branch
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.
Description
Re-applies switch to MR UI in order to conditionally toggle MUI theme on / off.
How Has This Been Tested?
Toggle switch in page masthead and verify that the MUI theme is toggled on / off based on the value of the switch.
Tests N/A, visual updates only.
Merge criteria:
DCOcheck)If you have UI changes