Skip to content

fix(storybook): fix theme picker current selection #25533

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.

Sign up for GitHub

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

Merged
tudorpopams merged 1 commit into from
Nov 9, 2022
Merged

fix(storybook): fix theme picker current selection #25533

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions apps/public-docsite-v9/src/DocsComponents/ThemePicker.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,18 @@ const useStyles = makeStyles({
*/
export const ThemePicker: React.FC<{ selectedThemeId?: string }> = ({ selectedThemeId }) => {
const styles = useStyles();
const [currentThemeId, setCurrentThemeId] = React.useState(selectedThemeId ?? null);
Copy link
Contributor

@Hotell Hotell Nov 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dig a bit deeper and it looks like that const context = React.useContext(DocsContext); within FluentDocsPage.stories doesn't work at all - it has no context property. To properly fix this we should fix the issue over there as this is moreless stateless component that is being set by parent.

More context:

DocsContext.globals exposes only following map

{
measureEnabled:  false,
outline:  false
}

Copy link
Contributor

@Hotell Hotell Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quickly looking at others custom controls we have and it seems we might not have another option. TOC is doing the same thing.

Let's remove { selectedThemeId?: string } prop API from ThemePicker as its not being set anyways and handle it internally.

Copy link
Contributor Author

@tudorpopams tudorpopams Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the theme property is set in the global context after the emit, but because we can't really listen to those changes in an elegant way we still neet that selectedThemeId prop. This makes the dropdown keep the same value when switching from one component to another (ie change the theme on <Button>, then click on <CompoundButton>).

Copy link
Contributor

@Hotell Hotell Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is set, but DocsPage wont get those values from globals thus this code doesn't do anything

image

This is the change I had in mind + applying your changes in this PR
MicrosoftTeams-image

Copy link
Contributor Author

@tudorpopams tudorpopams Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what I mean is that the FluentDocsPage example where ThemePicker passes that selectedTheme.id value to the prop is actually working after the addons.getChannel().emit('updateGlobals', { globals: { [THEME_ID]: themeId } }); call. This is the mechanism we use to keep the theme name consistent in the dropdown when changing pages.


const setGlobalTheme = (themeId: ThemeIds): void => {
addons.getChannel().emit('updateGlobals', { globals: { [THEME_ID]: themeId } });
};
const onCheckedValueChange: MenuProps['onCheckedValueChange'] = (e, data) => {
setGlobalTheme(data.checkedItems[0] as ThemeIds);
const newThemeId = data.checkedItems[0] as ThemeIds;
setGlobalTheme(newThemeId);
setCurrentThemeId(newThemeId);
};

const selectedTheme = themes.find(theme => theme.id === selectedThemeId);
const selectedTheme = themes.find(theme => theme.id === currentThemeId);

return (
<Menu
Expand Down