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

[Storybook] fix warning and improve perf #407

Merged
merged 3 commits into from
Oct 7, 2020
Merged

Conversation

dtassone
Copy link
Member

@dtassone dtassone commented Oct 7, 2020

No description provided.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Without a description of the pull request, I can't know what I should review.

@@ -24,10 +24,11 @@ module.exports = {
'@storybook/addon-a11y/register',
],
typescript: {
check: __DEV__, // Netlify is breaking the deploy with this settings on. So deactivate on release
check: __DEV__, // Netlify is breaking the deploy with this settings on. So deactivate on release
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, this should be formated by prettier but isn't.

@eps1lon By default, we ignore dot files/folders, shouldn't set dot: true in the glob options?

Copy link
Member

@eps1lon eps1lon Oct 7, 2020

Choose a reason for hiding this comment

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

For prettier that makes sense, yes.

Edit: Probably makes sense for almost anything now that I think about it. Usually you want to ignore vendored files from these tasks. But not every dotfile is vendored. The only dotfile that is vendored is .yarn if I recall correctly.

Copy link
Member

@oliviertassinari oliviertassinari Oct 7, 2020

Choose a reason for hiding this comment

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

Ok, I will add it in the next batch of small changes. It should be a one-line change. Then, once committed we can benefit from it on material-ui-x.

Copy link
Member

Choose a reason for hiding this comment

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

It should be a one-line change.

Those are usually the kind of changes the lead to days of work. At least I thought so in mui/material-ui#22814: Just bump the browser version and done 😆

packages/storybook/.storybook/main.js Outdated Show resolved Hide resolved
@@ -1,5 +1,4 @@
import * as React from 'react';
import { withA11y } from '@storybook/addon-a11y';
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean that we should remove @storybook/addon-a11y from the list of dependencies?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, we just don't need to declare it here

@dtassone
Copy link
Member Author

dtassone commented Oct 7, 2020

Without a description of the pull request, I can't know what I should review.

As mentioned, this PR fix all the warnings and improve the SB build time

@dtassone
Copy link
Member Author

dtassone commented Oct 7, 2020

70+ sec down to 30 sec

@oliviertassinari
Copy link
Member

I have recently found https://vitro.now.sh/. I wonder if we shouldn't consider it as an alternative to Storybook. Vitro comes with Next.js built-in. Maybe it could nail the performance issue of Storybook. I mean: check this benchmark: https://www.webpagetest.org/video/compare.php?tests=201007_DiKP_23fb114e7e169261238c212e78d5b336,201007_DiBM_e569507ebd60a8b9e5075b8b479b3a07

Capture d’écran 2020-10-07 à 15 25 34

Something could be improved

@dtassone
Copy link
Member Author

dtassone commented Oct 7, 2020

I have recently found https://vitro.now.sh/. I wonder if we shouldn't consider it as an alternative to Storybook. Vitro comes with Next.js built-in. Maybe it could nail the performance issue of Storybook. I mean: check this benchmark: https://www.webpagetest.org/video/compare.php?tests=201007_DiKP_23fb114e7e169261238c212e78d5b336,201007_DiBM_e569507ebd60a8b9e5075b8b479b3a07

Capture d’écran 2020-10-07 à 15 25 34

Something could be improved

Still very early stage
No Addon like Knobs and all the others in the pipeline

@dtassone dtassone merged commit 74d4fc9 into mui:master Oct 7, 2020
@oliviertassinari
Copy link
Member

Yeah, it's probably too early.

dtassone added a commit to dtassone/material-ui-x that referenced this pull request Nov 9, 2020
* fix storybook perf and warning

* fix perf on sourcemaps

* improve build time
@oliviertassinari oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants