Skip to content
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.

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

Fix Storybook incompatibility with Tailwindcss (fixes #62) #74

Merged
merged 18 commits into from
Oct 14, 2020
Merged

Fix Storybook incompatibility with Tailwindcss (fixes #62) #74

merged 18 commits into from
Oct 14, 2020

Conversation

Iinh
Copy link
Contributor

@Iinh Iinh commented Oct 8, 2020

This PR fixes issue #62 by adding the correct Tailwind CSS components, using https://github.com/jerriclynsjohn/svelte-storybook-tailwind as template.

Screen Shot 2020-10-08 at 7 32 16 AM

Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

Hi @linh! Thank you, this looks much better. However it looks like the heading isn't getting styled as I'd expect (it should be larger). I think this is because the styles in https://github.com/mozilla/glean-dictionary/blob/main/src/Tailwindcss.svelte aren't getting imported. Could you see if there's a way for them to be imported by default for all stories? You'll see that the main application includes them like this:

<Tailwindcss />

@Iinh
Copy link
Contributor Author

Iinh commented Oct 12, 2020

What I’ve tried

I imported Tailwind to the story file itself to use as a styling wrapper with decorators. However, the problem is custom decorator syntax requires a div with literal styling that wraps around the component, like so:

export const Basic = () => {
  Component: SchemaViewer,
  decorators:  [(Story) => <div style={{ margin: '3em' }}><Story/></div>]
};

Tailwindcss.svelt is a Svelte component, so it can’t be properly used inside the callback function. Something like this wouldn’t work:

Import Tailwindcss from “../Tailwind”
export const Basic = () => {
  Component: SchemaViewer,
  decorators:  [(Story) => <div style={{ Tailwindcss }}><Story/></div>]
};

My solution

Currently, Tailwindcss is compiled to a separate CSS file at run time. The output of this file, according to the rollup configuration, is at /public/build/bundle.css. Since this CSS file always has to be compiled to be used in the main app, I thought why not use it for Storybook too. This also ensures that any additional styling, once processed, will apply to all stories. Attached image shows how the story looks with this solution.

Looking forward to feedback. Thank you!

story

@wlach
Copy link
Contributor

wlach commented Oct 12, 2020

@linh Would this not work?

Import Tailwindcss from “../Tailwind”
export const Basic = () => {
  Component: SchemaViewer,
  decorators:  [(Story) => <div><Tailwindcss/><Story/></div>]
};

Using the bundle css directly might be ok, though I'd worry about it being kept up to date (I'm not sure if storybook will recompile it when it changes?).

Regardless, thanks for your persistence on this!

@Iinh
Copy link
Contributor Author

Iinh commented Oct 13, 2020

Hi @wlach, your code would cause an error in storybook. I believe because you used JSX (<div><Tailwindcss/><Story/></div>) inside decorators, and JSX syntax is invalid in Javascript without a compiler. That’s why the code sample used in the document for decorators would not work in our case because they all use JSX. However it would work in a React application.

The reason why I went with my solution because to fix this bug, the goal was to import the styling currently in Tailwindcss.svelte to storybook. After realizing that I could not use decorators or import the Tailwind component directly, I figured that another way to achieve this goal was to import the css bundle.

For now it does the job. Any changes to bundle.css that directly affects storybook will apply. I agree with you that it’s not the best way as we have to always make sure that css bundle is up to date when making Tailwind changes. As we add more stories that require complicated styling, to avoid relying on css bundle, we could:

  • Have a separate styling folder system from the main app for better separation of concerns
  • Set up Postcss configuration inside storybook so we don’t have to run npm run dev and rely on development build for Tailwind processing, something similar to this.

What do you think? I appreciate your time and feedback!

@wlach
Copy link
Contributor

wlach commented Oct 13, 2020

Hi @wlach, your code would cause an error in storybook. I believe because you used JSX (<div><Tailwindcss/><Story/></div>) inside decorators, and JSX syntax is invalid in Javascript without a compiler. That’s why the code sample used in the document for decorators would not work in our case because they all use JSX. However it would work in a React application.

You are of course totally right on this. Apologies for the misdirection.

Set up Postcss configuration inside storybook so we don’t have to run npm run dev and rely on development build for Tailwind processing, something similar to this.

I think this sounds like the way to go! The main piece of functionality we're looking for here are the storybook components reloading when people modify the top-level tailwind component. If that works, I think we're good to merge.

Thanks so much for your persistence on this.

@Iinh Iinh changed the base branch from main to dependabot/npm_and_yarn/postcss-load-config-3.0.0 October 14, 2020 09:53
@Iinh Iinh changed the base branch from dependabot/npm_and_yarn/postcss-load-config-3.0.0 to main October 14, 2020 09:54
@Iinh
Copy link
Contributor Author

Iinh commented Oct 14, 2020

This update is looking to fix 2 problems.

1) Tailwind incompatibility with Storybook

Currently Tailwind doesn't work properly in Storybook. This is fixed by updating the webpack configuration to include svelte-preprocess. However this fix alone doesn't completely resolve the issue of storybook not rendering the top-level tailwind component because of the problem below.

2) Global styles not available globally

Tailwindcss.svelte has all css global rules, including the Tailwind utilities packages. The problem with this file is it's placed inside App.svelte, which means that global styles only get compiled and become available for other components after App is called. This is fine in development as App is always the first one called. However, in storybook, each component is called in isolation. For example, when SchemaViewer loads in Storybook the only styling available is from SchemaViewer itself. If App was never called, SchemaViewer can't inherit any global styles.

To fix this problem, we need to keep global styles in a different file. 3 main requirements:

  • It can’t be a svelte component because we can’t use component syntax for styling in storybook
  • It needs to be accessible to all components: it can’t belong to just one component, like how inaccessible Tailwindcss.svelte was because it's only available through App
  • Changes to this file need to reload the components, both in development and storybook

I believe the best option is to replace Tailwindcss.svelte with a css file for global styles, because:

  • A css file can be easily imported to use with storybook. Any changes will directly reload the stories
  • In development: svelte-preprocess doesn’t compile css files outside of Svelte components, we can use postcss-cli to compile it instead. This css file writes to public/build/global.css, which is then used in index.html -- global styles will apply to all components. An npm script was added to watch this css file so that it'll recompile if changes are added.

With this change, all stories will independently inherit global styles. We also don't have to rely on development build as the css file is imported directly to storybook.

Looking forward to feedback @wlach. Thank you!

@Iinh Iinh changed the title Add missing Tailwind CSS imports to storybook Fix Storybook incompatibility with Tailwindcss Oct 14, 2020
Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

So in storybook, this works great! Unfortunately we lose live reloading inside the dev environment. I think what we need is a hybrid approach: the current approach in storybook, and the component approach in the actual app (by importing the css file inside of it, instead of defining it inline). I just tested and this seems possible, but I'll leave the solution to you.

This guide will be helpful for figuring it out: https://www.apostrof.co/blog/getting-tailwind-css-to-integrate-nicely-with-svelte-sapper/

Extra hint: You'll need to add postcss-import.

package.json Outdated
@@ -4,7 +4,7 @@
"scripts": {
"build": "rollup --config",
"build-storybook": "build-storybook --static-dir ./public --config-dir .storybook",
"dev": "rollup --config --watch",
"dev": "rollup --config --watch & postcss src/tailwind.css -o public/build/global.css -w",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be necessary (see review)

@Iinh
Copy link
Contributor Author

Iinh commented Oct 14, 2020

Hi @wlach thank you for your feedback! I've updated my code with the hybrid approach. Looks like CircleCI failed my PR because of some code format error in Tailwindcss.svelte. I tried changing it around but it didn't seem to work. Locally my linter is not complaining anything so it's not clear what I need to fix either, I probably need another pair of eyes to help find this format error. What usually is your approach to solve issues like this? Thank you.

ps Sorry about the messy commit history, I was changing the file around hoping it would pass Circleci.

Format code

Change css file name

Fix linter error for circleci
@wlach
Copy link
Contributor

wlach commented Oct 14, 2020

@linh try running npm run format! It should autofix things for you

@Iinh
Copy link
Contributor Author

Iinh commented Oct 14, 2020

@wlach YES that fixed it, thank you so much! I learned so many new things this week. This PR is ready for review.

@wlach wlach changed the title Fix Storybook incompatibility with Tailwindcss Fix Storybook incompatibility with Tailwindcss (fixes #62) Oct 14, 2020
Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

Awesome!

@wlach wlach merged commit 2b5d98a into mozilla:main Oct 14, 2020
JoyLubega pushed a commit to JoyLubega/glean-dictionary that referenced this pull request Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants