feat: migrate theme popover to v9

[Mnickii]

Overview

Brief description of what this PR does, and why it is needed.
Closes #3439
Closes #3383

Demo

Optional. Screenshots, curl examples, etc.

Notes

Optional. Ancillary topics, caveats, alternative strategies that didn't work out, anything else.

Testing Instructions

• How to test this PR
• Prefer bulleted description
• Start after checking out this branch
• Include any setup required, such as bundling scripts, restarting services, etc.
• Include test case, and expected output

[musale]

Fantastic work so far!

There's a console warning that draws attention to the implementation you have here

My understanding is that you can't use defaultValue and checked at the same time. My recomendation here is to:

1. use the onChange listener on RadioGroup and remove the defaultValue for value.
2. You can then create a state value which defaults to the appTheme, and use all the implementation of handleChangeTheme after setValue(data.value). Something like this:

 const [value, setValue] = useState(appTheme);
// some stuff
 <RadioGroup value={value} onChange={(_, data) =>{
        const newTheme: string = data.value
        setValue(newTheme)
        // Applies the theme to the Fluent UI components
        dispatch(changeTheme(newTheme));
        loadGETheme(newTheme); //Remove when cleaning up
        telemetry.trackEvent(eventTypes.BUTTON_CLICK_EVENT, {
          ComponentName: componentNames.SELECT_THEME_BUTTON,
          SelectedTheme: newTheme.replace('-', ' ').toSentenceCase()
        });
      }}>
// other stuff

3. Remove checked and onClick on Radio.

Borrowed this from https://react.fluentui.dev/?path=/docs/components-radiogroup--docs#controlled-value

[Mnickii]

I'm currently unable to test this locally, but I'm curious if removing the Radio Checked on ln 71 in current implementation would have a similar warning. Per https://react.fluentui.dev/?path=/docs/components-radiogroup--docs#default-value, only either the default Value or the checked can be used

[ElinorW]

Noticed the close button is no longer there, shouldn't we still keep it? but just as a regular button?

[Mnickii]

@ElinorW since clicking outside the popover closes it, I wonder if we should keep it.

[ElinorW]

@ElinorW since clicking outside the popover closes it, I wonder if we should keep it.

Hmm good point, the same behaviour is there in the current version as well.
Maybe we can ask design.

[musale]

Since you've added system theme support, make it an option and probably the default selected.

[Mnickii]

It's the default selected ln70-75

[musale]

I meant on the popup

[musale]

In App.tsx, we want to do two things:

• Get the current theme or load the system theme and set it. Allows us to set the theme on app load.
• Create a listener to the theme changes. Allows us to change our theme if a user changes the theme from their system. Something like this:

  const mediaQueryList = window.matchMedia('(prefers-color-scheme: dark)');
  // Add the listener for theme changes
  mediaQueryList.addEventListener('change', handleThemeChange);

What do you think?

[Mnickii]

Yeah @musale, I noticed the theme was only picked when interacting with theme chooser, currently implementing this.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud