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 autoprefixer with storybook #3

Merged
merged 6 commits into from
Jul 29, 2019

Conversation

nickofthyme
Copy link

Summary

  • Upgraded to storybook@5 to see if that would resolve the issue but it didn't 😞. But I figured we keep the changes anyway.
  • Updated webpack.config.js to correct lazy-loading issue with storybook and autoprefixer.

@markov00 & @emmacunningham Can you check my changes to make sure it doesn't break anything?

The only thing I noticed was that with storybook v5 the story tab next to the knobs tab that shows the code of the story, was VERY VERY slow. I'm thinking this is just due to all the stories being in the same file, but not sure.

Screenshot

Image below shows the legend in storybook correctly prefixes mask-image under the echLegendListContainer class.

image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

@nickofthyme
Copy link
Author

Related to elastic#267

@markov00
Copy link
Owner

Thanks @nickofthyme for that, I was trying to upgrade to storybook 5 few times in the past but there was always some some problems on the proptypes not rendered (that are still not rendered in our storybook v4 :( )
What do you think of one of the following ways to solve the slow speed of the source panel:

  • upgrade to use 5.2 beta storybook that has a better source loader and split each story into multiple files: I've done a test with:
import { storiesOf } from '@storybook/react';
import * as basicExample from './basic';
import * as multiAxis from './multi_axis';
storiesOf('Grids', module)
  .add(basicExample.title, basicExample.story)
  .add(multiAxis.title, multiAxis.story);

// and each file can have something like the following

export const title = 'Basic';
export function story() {
  • disable the storysource addon and enable the source in the info addon (that is useful in a way because it shows a more useful code (it execute/substitute every function the the JSX code) but it removes also some required calls like all the const properties

@markov00
Copy link
Owner

@nickofthyme I've also another request: can we keep the original scss file name without useable postfix? I know that currently no one is using that scss directly (only the theme_only one), but I'd like to avoid changing that name because of those imports.
Maybe it's worth trying using the resourceQuery option on the loader https://webpack.js.org/configuration/module/#ruleresourcequery

@markov00
Copy link
Owner

I think you can use the following:

  config.module.rules.push({
    test: /\.scss$/,
    oneOf: [
      {
        resourceQuery: /useable/,
        use: [
          {
            loader: 'style-loader/useable',
            options: {
              attrs: {
                nonce,
              },
            },
          },
          ...scssLoaders,
        ],
      },
      {
        use: [
          {
            loader: 'style-loader',
            options: {
              attrs: {
                nonce,
              },
            },
          },
          ...scssLoaders,
        ]
      }
    ]
  });

and change the import of the theme_service.ts to

import themeDark from '../src/theme_dark.scss?useable';

@nickofthyme
Copy link
Author

nickofthyme commented Jul 29, 2019

@markov00 I made the fixes you recommended including...

  • Using resourceQuery to designate lazy-loaded .scss files. I used the ?lazy query cuz that is what it is actually doing rather than the usable query.
  • I chose to disable the storysource addon in favor of the source option. I tried to split the stories into separate files but the source is pulled from where the storiesOf function is invoked so all you would see is something like below 👇 which obfuscates all the code and renders it pretty useless. I also like with the source option that you have the option to copy the code.
storiesOf('Grids', module)
  .add(basicExample.title, basicExample.story)
  .add(multiAxis.title, multiAxis.story);

I also tried to upgrade to v5.2.0-beta.X but they upgraded their code to typescript between v5.1.9 and v5.2.0-beta.X causing a lot of type issues that I've created issues for.

Screenshots

image

Copy link
Owner

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

LGTM!

package.json Outdated
"@types/storybook__addon-actions": "^3.4.3",
"@types/storybook__addon-info": "^4.1.2",
"@types/storybook__addon-knobs": "^5.0.3",
"@types/storybook__addon-options": "^4.0.2",
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should remove this type as we already removed addon-options.

@markov00 markov00 merged this pull request into markov00:update_eui Jul 29, 2019
@nickofthyme nickofthyme deleted the fix/autoprefixer branch July 29, 2019 21:27
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.

2 participants