Skip to content

Conversation

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 30, 2016

  • I have changed the stateless functionnal component organisation because of Add a failing test for sort-prop-types jsx-eslint/eslint-plugin-react#567.
  • Do you think I should be writing more tests for this PR?
  • I had to add one feature to the EventListener component.
  • Deprecate the styleResizable mixin and remove it from the documentation. That's was our last mixin #RIP. Now, we should be able to enforce react/prefer-es6-classes.
  • Remove usage of material-ui/lib from the documentation.

@oliviertassinari
Copy link
Member Author

@callemall/material-ui There are unrelated topics inside this PR. If you think this chunk is too big. I can break it down.

@newoga
Copy link
Contributor

newoga commented Apr 30, 2016

@newoga I have changed the stateless functionnal component organisation because of jsx-eslint/eslint-plugin-react#567.

That's fine by me! As long as we're consistent 😄

Deprecate the styleResizable mixin and remove it from the documentation. That's was our last mixin #RIP. Now, we should be able to enforce react/prefer-es6-classes.

🎉 Finally! 🎉

@nathanmarks
Copy link
Contributor

@oliviertassinari doing a PR for that right now

@newoga
Copy link
Contributor

newoga commented Apr 30, 2016

Should we put the withWidth HoC in util instead of style. Conceptually I always thought of util as our internal files and components, and style and the public util. If I'm correct thinking that way, then maybe util is a better home for it.

Thoughts?

*/
style: PropTypes.object,
width: PropTypes.string.isRequired,
width: PropTypes.number.isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

Tabs documentation page -- I'm getting this warning in my console now:

warning.js:44 Warning: Failed propType: Invalid prop `width` of type `string` supplied to `InkBar`, expected `number`. Check the render method of `Tabs`.

@newoga
Copy link
Contributor

newoga commented Apr 30, 2016

Now, we should be able to enforce react/prefer-es6-classes.

@oliviertassinari Should we just do that as part of this PR?

@mbrookes
Copy link
Member

mbrookes commented May 1, 2016

Should we put the withWidth HoC in util instead of style. Conceptually I always thought of util as our internal files and components, and style and the public util. If I'm correct thinking that way, then maybe util is a better home for it.

Yes, internal is internal components, utils internal utilities, and styles (mostly) public style / theme stuff.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 1, 2016

@oliviertassinari Should we just do that as part of this PR?

Done! I have

  • enforced react/prefer-es6-class
  • moved withWidth HOC to the /utils folder
  • Added two small test
  • Split the PR into 4 distinct commits.

@newoga
Copy link
Contributor

newoga commented May 2, 2016

@oliviertassinari This looks good to me. Nice commits!

@mbrookes
Copy link
Member

mbrookes commented May 2, 2016

Nice work @oliviertassinari! 💯

@oliviertassinari oliviertassinari merged commit 2ee7024 into mui:master May 2, 2016
@oliviertassinari oliviertassinari deleted the with-width-hoc branch May 2, 2016 16:54
@damonmaria
Copy link

@oliviertassinari: I was going to use this new component but the widths used don't match the Material Design window sizes. Is there are reason they are different?

@oliviertassinari
Copy link
Member Author

@damonmaria Good question, Material-UI completely overlook the responsive aspect of the specification. In this PR, I was focusing on porting the mixin into a high order component.
We definitely need to improve this breaking points 👍.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants