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

[core] Remove Storybook #6040

Merged
merged 13 commits into from
Sep 9, 2022
Merged

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Sep 6, 2022

We discussed the Storybook yesterday: https://mui-org.notion.site/xGrid-Weekly-meeting-2022-09-05-931b28fff95b43d4905f328a884076b1.

It has received almost no new story in a year and is therefore totally outdated.
Right now, it is only an overhead that we have to maintain.

  • Move the useData and getData utilities that are being used in tests from the storybook to @mui/x-data-grid-generator
  • Remove the storybook internal package
  • Remove the Storybook from regression tests
  • Remove the Storybook from the CI and from our architecture files

@flaviendelangle flaviendelangle added the core Infrastructure work going on behind the scenes label Sep 6, 2022
@flaviendelangle flaviendelangle self-assigned this Sep 6, 2022
@mui-bot
Copy link

mui-bot commented Sep 6, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 464.1 1,024.2 478.8 653.72 203.103
Sort 100k rows ms 548.1 916.9 894.6 785.42 152.878
Select 100k rows ms 182.5 262.6 215.1 221.58 27.086
Deselect 100k rows ms 152.7 279.9 183.5 202.28 46.816

Generated by 🚫 dangerJS against 0591dad

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.

It sounds good.

(I have linked the meeting notes that talk about the change and made them public for future/community scrutiny.)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 6, 2022
@github-actions
Copy link

github-actions bot commented Sep 6, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 6, 2022
@@ -24,6 +24,10 @@ module.exports = {
new webpack.DefinePlugin({
DISABLE_CHANCE_RANDOM: JSON.stringify(true),
}),
new webpack.ProvidePlugin({
Copy link
Member Author

Choose a reason for hiding this comment

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

Copied from the core

https://github.com/mui/material-ui/blob/c8feb29320a22cc4461fb5ef5f4f7de7bd96a59b/test/regressions/webpack.config.js#L26
I think the Storybook was injecting a default config somehow.

rows: GridBasicRowModel[];
}

export const getBasicGridData = (rowLength: number, colLength: number): GridBasicData => {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I guess this was part of the storybook package. I wanter if we can remove it altogether and just use either the Commodity or Employee data sets. If there are no assertions in the tests related to the value of the cells then replacing the data set should be more or less easy, what do you think?

Copy link
Member Author

@flaviendelangle flaviendelangle Sep 6, 2022

Choose a reason for hiding this comment

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

Yep, it's the old useData and getData.

For the tests, it can be useful to have those dummy data tests without any intelligence behind them.
For instance in the pagination, it makes very clear what you are testing.

I would personally be in favor of an even dumber data set.
Something like:

{ id: 0, name: `name-0` },
{ id: 1, name: `name-1' },
...

And to use it everywhere we have some "Nike" / "Puma" dataset.
To reduce as much as possible the complexity of understanding the dataset and focusing on the actual behavior we are testing.

@flaviendelangle flaviendelangle marked this pull request as ready for review September 7, 2022 07:43
</div>
);
};
export const MultilineCustomCellContentSnap = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Since the Argos CI step is broken, we don't have visibility on the scale of this, but here is one visual test that is gone. Will we miss these visual regression tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Here are all the stories with the Snap suffix
If the team wants to migrate some to a hidden doc example I can do it
I personally don't see any good candidate.

Copy link
Member

Choose a reason for hiding this comment

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

I personally don't see any good candidate.

Then I think that we are good

to a hidden doc example

In the main repo, we store them here https://github.com/mui/material-ui/tree/master/test/regressions/fixtures (outside of the docs)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I think we had pickers fixtures that I did not migrate 😄
I'll have a look to maybe bring back this logic later

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 8, 2022
@github-actions
Copy link

github-actions bot commented Sep 8, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 8, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 9, 2022
@github-actions
Copy link

github-actions bot commented Sep 9, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 9, 2022
@flaviendelangle flaviendelangle merged commit e15560c into mui:master Sep 9, 2022
@flaviendelangle flaviendelangle deleted the storybook branch September 9, 2022 10:15
@flaviendelangle flaviendelangle restored the storybook branch September 9, 2022 10:16
flaviendelangle added a commit that referenced this pull request Sep 9, 2022
flaviendelangle added a commit that referenced this pull request Sep 9, 2022
Revert "[core] Remove Storybook (#6040)"

This reverts commit e15560c.
oliviertassinari pushed a commit to oliviertassinari/mui-x that referenced this pull request Sep 9, 2022
Revert "[core] Remove Storybook (mui#6040)"

This reverts commit e15560c.
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Sep 12, 2022
Revert "[core] Remove Storybook (mui#6040)"

This reverts commit e15560c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants