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.

Sign up for GitHub

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

Jump to bottom

refactor storybook and add first keyboard test #36

Merged
merged 6 commits into from Jun 29, 2020
Merged

refactor storybook and add first keyboard test #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b7cc87d
refactor storybook and added first keyboard test
dtassone Jun 25, 2020
622497d
lint and prettier
dtassone Jun 25, 2020
3f11f2e
fix issue with stories and rollback change on totalHeight
dtassone Jun 25, 2020
3ee11d2
Merge branch 'master' into image-snap
dtassone Jun 26, 2020
8a897ea
refactor keyboard test and add components
dtassone Jun 29, 2020
3d8e4fc
lint and prettier
dtassone Jun 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Expand Up @@ -7,3 +7,4 @@
*.log
dist
node_modules
__diff_output__
29,842 changes: 0 additions & 29,842 deletions package-lock.json
View file
Open in desktop

This file was deleted.

5 changes: 4 additions & 1 deletion package.json
View file
Open in desktop
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
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion packages/grid/data-grid/README.md
View file
Open in desktop
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

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

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