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

refactor storybook and add first keyboard test #36

Merged
merged 6 commits into from
Jun 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@
*.log
dist
node_modules
__diff_output__
29,842 changes: 0 additions & 29,842 deletions package-lock.json

This file was deleted.

5 changes: 4 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
"author": "Damien Tassone",
"private": true,
"devDependencies": {
"@material-ui/icons": "^4.9.1",
"@testing-library/jest-dom": "^4.2.4",
"@testing-library/react": "^9.3.2",
"@testing-library/user-event": "^7.1.2",
Expand All @@ -18,6 +17,10 @@
"@types/styled-components": "^5.0.1",
"@typescript-eslint/eslint-plugin": "^2.19.0",
"@typescript-eslint/parser": "^2.19.0",
"@material-ui/core": "^4.9.12",
"@material-ui/icons": "^4.9.1",
"@material-ui/lab": "^4.0.0-alpha.54",
"styled-components": "^5.1.0",
"enzyme": "^3.11.0",
"enzyme-adapter-react-16": "^1.15.2",
"eslint": "^6.8.0",
Expand Down
6,042 changes: 6,042 additions & 0 deletions packages/demo-app/yarn.lock

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion packages/grid/data-grid/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ This component has 2 peer dependencies that you will need to install as well.
```
"peerDependencies": {
"react": "^16.13.1",
"styled-components": "^5.0.1"
"styled-components": "^5.1.0"
Copy link
Member

@oliviertassinari oliviertassinari Jun 25, 2020

Choose a reason for hiding this comment

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

This doesn't seem right.

https://github.com/mui-org/material-ui-x/blob/6ded9f9c625b14be75289119153430b75b4a4a19/packages/grid/data-grid/package.json#L26-L30

@material-ui/core already has a CSS-in-JS solution, introducing a second means duplication of bundle size and configuration. I think that we should stick to using @material-ui/styles until we figure out the way out of this with @mnajdova in v5.

Note that it doesn't prevent end-users to use styled-components.

Copy link
Member Author

Choose a reason for hiding this comment

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

Styled components is one of the most popular solution now. Css in js is much less practical. If you move away of this framework in v5, I think it is a waste of time to realign it now, and change it again later. Don't you think?
It is a peer dependency so it won't be bundled. Happy to refactor it and use css modules with less

Copy link
Member

Choose a reason for hiding this comment

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

Styled components is one of the most popular solution now.

And yet, it's not the most popular one in the first userbase we are going after with this MIT version: https://deploy-preview-21555--material-ui.netlify.app/blog/2020-developer-survey-results/#19-what-styling-system-are-you-using

It is a peer dependency so it won't be bundled.

What happen if the peer dependency is missing? Does it crash?
If it doesn't crash, why do we need to include it? If it crashes, it will be bundled.

I think it is a waste of time to realign it now, and change it again later. Don't you think?

No strong preference, I'm curious to see how this will play out. I wanted to raise the concern for the record, so we can come back to it in 6 months and reevaluate.

Copy link
Member Author

Choose a reason for hiding this comment

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

The main reason for adding it as peer dependency is to allow the consumer to control the version of the package and avoid having several versions of the same package in the components tree which usually have disastrous consequences if packages interact with each other like for react.
TBH I'm not a big fan of having any dependencies beside react, let see if we can improve that in the future ;)

Copy link
Member

@oliviertassinari oliviertassinari Jun 26, 2020

Choose a reason for hiding this comment

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

The main reason

Users can control the version of the dependency even if it's not a peer dependency. The reason has to be found somewhere else. It's the same as why react and react-dom should be peer dependencies: they have a singleton. SC explains a bit more why in https://styled-components.com/docs/faqs#i-am-a-library-author-should-i-bundle-styledcomponents-with-my-library and https://styled-components.com/docs/faqs#why-am-i-getting-a-warning-about-several-instances-of-module-on-the-page.

TBH I'm not a big fan of having any dependencies beside react, let see if we can improve that in the future ;)

💯 so much agree here, I hope we can take this problem down in v5, it will both help for users asking for better bundle size (no duplication), users asking for easier customization (no need to configure the CSS injection order with another tool), help with the marketing perceived value (you are in control, it's light).

},
```

Expand Down
Loading