feat(ui): Storybook - Add theme switcher & noBorder option to <Sample /> #29765

vuluongj20 · 2021-11-03T23:39:43Z

Adding two optional props to <Sample />:

noBorder: remove border and padding around the sample wrapper
showThemeSwitcher: add a toggle for switching between light and dark mode. Sample now provides a local theme context to automatically update the styles of all of its children.

Without noBorder:

With noBorder:

With showThemeSwitcher enabled:

vuluongj20 · 2021-11-03T23:41:15Z

static/app/icons/iconMoon.tsx

+
+IconMoon.displayName = 'IconMoon';
+
+export {IconMoon};


IconMoon is already part of the icon set in Figma. I just copied the SVG markup from there.

github-actions · 2021-11-03T23:42:14Z

size-limit report

Path	Base Size (9a5c1a2)	Current Size	Change
src/sentry/static/sentry/dist/entrypoints/app.js	52.76 KB	52.77 KB	+0.02% 🔺
src/sentry/static/sentry/dist/entrypoints/sentry.css	70.89 KB	70.89 KB	0%

vuluongj20 · 2021-11-03T23:44:38Z

docs-ui/components/sample.tsx

+};
+
+/** Expose the selected theme to children of <Sample /> */
+export const SampleThemeContext = createContext<ThemeName>('light');


Exporting SampleThemeContext so children of Sample /> can consume it (via useContext(SampleThemeContext)) for their own purposes.

Hmm why would children need to use this value?

In the Colors page that I'm working on, there are color samples for both light and dark mode:

These color samples need to show different values based on the current theme in <Sample />, e.g. Gray 500 shows #2B2233 in light mode and #EBE6EF in dark mode. This means they need to know which theme (light/dark) has been selected, hence the theme context.

Ahh... are we going to have a separate palette for dark and light modes? I think the initial plan was to have a single palette (but not sure if that's still the plan)

Yes, the plan is to have separate palettes 🌈

Adding two optional props to <Sample />: - noBorder: remove border and padding around the wrapper - showThemeSwitcher: add a toggle for switching between light and dark mode. Sample now provides a local theme context to automatically update the styles of all of its children.

billyvg · 2021-11-04T20:19:39Z

docs-ui/components/sample.tsx

+  showThemeSwitcher?: boolean;
+  noBorder?: boolean;


Might be nice to add some /** */ comments here

billyvg · 2021-11-04T20:22:10Z

docs-ui/components/sample.tsx

+    themeObject = theme === 'light' ? lightTheme : darkTheme;
+  } else {
+    themeObject = useTheme();


You probably want themeObject to be in state, it probably works fine most of the time because you call setTheme, but this could lead to unwanted behavior because it's not in state.

billyvg · 2021-11-04T20:22:28Z

docs-ui/components/sample.tsx

+  return (
+    <Wrap>
+      {showThemeSwitcher && (
+        <ThemeSwitcher onClick={() => toggleTheme()} active={theme === 'dark'}>


Suggested change

<ThemeSwitcher onClick={() => toggleTheme()} active={theme === 'dark'}>

<ThemeSwitcher onClick={toggleTheme} active={theme === 'dark'}>

billyvg · 2021-11-04T20:24:38Z

docs-ui/components/sample.tsx

+};
+
+/** Expose the selected theme to children of <Sample /> */
+export const SampleThemeContext = createContext<ThemeName>('light');


Hmm why would children need to use this value?

@billyvg

Based on @billyvg's comments in #29765.

vuluongj20 · 2021-11-05T19:29:40Z

@billyvg Added a commit addressing your points above. Tell me if it looks good (esp. how I added themeObject to state)!

billyvg

Codewise, looks good

billyvg · 2021-11-09T21:28:46Z

docs-ui/components/sample.tsx

+};
+
+/** Expose the selected theme to children of <Sample /> */
+export const SampleThemeContext = createContext<ThemeName>('light');


Ahh... are we going to have a separate palette for dark and light modes? I think the initial plan was to have a single palette (but not sure if that's still the plan)

billyvg · 2021-11-09T21:30:11Z

docs-ui/components/sample.tsx

+      setThemeName('dark');
+      setTheme(darkTheme);


wonder if we should have the current theme name in theme itself... (I was avoiding it so that we wouldn't have logic based on theme name, and instead push people towards using theme aliases).

thought of this myself! Would definitely make this cleaner, but wasn't sure if it's worth changing theme.tsx for it.

vuluongj20 requested review from a team as code owners November 3, 2021 23:39

vuluongj20 commented Nov 3, 2021

View reviewed changes

vuluongj20 requested a review from billyvg November 3, 2021 23:45

vuluongj20 force-pushed the vuluong/storybook-theme-switcher branch from dc6d96f to 680a97c Compare November 3, 2021 23:49

vercel bot deployed to Preview – sentry November 3, 2021 23:49 View deployment

vercel bot deployed to Preview – storybook November 3, 2021 23:49 View deployment

billyvg reviewed Nov 4, 2021

View reviewed changes

Update sample.tsx

6ace564

Based on @billyvg's comments in #29765.

vercel bot deployed to Preview – sentry November 5, 2021 19:11 View deployment

vercel bot deployed to Preview – storybook November 5, 2021 19:11 View deployment

vuluongj20 requested a review from billyvg November 9, 2021 16:54

billyvg reviewed Nov 9, 2021

View reviewed changes

vuluongj20 requested a review from billyvg November 11, 2021 22:43

billyvg approved these changes Nov 12, 2021

View reviewed changes

vuluongj20 merged commit 96dd778 into master Nov 12, 2021

vuluongj20 deleted the vuluong/storybook-theme-switcher branch November 12, 2021 22:37

github-actions bot locked and limited conversation to collaborators Nov 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): Storybook - Add theme switcher & noBorder option to <Sample /> #29765

feat(ui): Storybook - Add theme switcher & noBorder option to <Sample /> #29765

vuluongj20 commented Nov 3, 2021

vuluongj20 Nov 3, 2021

github-actions bot commented Nov 3, 2021 •

edited

Loading

vuluongj20 Nov 3, 2021

billyvg Nov 4, 2021

vuluongj20 Nov 5, 2021

billyvg Nov 9, 2021

vuluongj20 Nov 10, 2021

billyvg Nov 4, 2021

vuluongj20 Nov 5, 2021

billyvg Nov 4, 2021

billyvg Nov 4, 2021

billyvg Nov 4, 2021

vuluongj20 commented Nov 5, 2021

billyvg left a comment

billyvg Nov 9, 2021

billyvg Nov 9, 2021

vuluongj20 Nov 10, 2021

	<ThemeSwitcher onClick={() => toggleTheme()} active={theme === 'dark'}>
	<ThemeSwitcher onClick={toggleTheme} active={theme === 'dark'}>


		IconMoon.displayName = 'IconMoon';

		export {IconMoon};

feat(ui): Storybook - Add theme switcher & noBorder option to <Sample /> #29765

feat(ui): Storybook - Add theme switcher & noBorder option to <Sample /> #29765

Conversation

vuluongj20 commented Nov 3, 2021

Choose a reason for hiding this comment

github-actions bot commented Nov 3, 2021 • edited Loading

size-limit report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vuluongj20 commented Nov 5, 2021

billyvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 3, 2021 •

edited

Loading