-
-
Notifications
You must be signed in to change notification settings - Fork 84
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
Port to Material UI V1 Beta36 #35
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good start! 👍 Inline styles should be changed to using withStyles
and JSX. Would you like to do that?
@@ -33,19 +33,22 @@ | |||
"babel-cli": "^6.14.0", | |||
"babel-core": "^6.8.0", | |||
"babel-eslint": "^7.2.3", | |||
"babel-plugin-direct-import": "^0.5.0", | |||
"babel-plugin-material-ui": "^0.3.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
babel-plugin-material-ui is not used anymore and can be removed
Took a quick look at this yesterday/today. I'm not sure I'll have time in the near future to do this properly (and styling is definitely my weakest area in front-end haha). It looks like consolidating the inline styles into a stylesheet/classname list that gets passed through withStyles() might be a little tough, especially the computed styles. Could probably make a bunch of separate style names (desktop-root, mobile-root, etc.), and select based on desktop/mobile/landscape. Might be worth waiting a little bit, after reading: mui/material-ui#8726 It sounds like they're looking into updating how withStyles() works to handle react-jss styles that are dynamic based on their props. Which would greatly simplify handling tweaking the styles based on desktop/mobile/landscape. If I'm overthinking this, and you had a different approach in mind, or can give me an example of how you'd want to port something non-trivial like: style={{
pointerEvents: this.props.open ? null : 'none',
opacity: this.props.open ? '1' : '0',
...this.props.style
}} to a withStyles() approach, hit me with it and I can take another look. If I do have some time this week, I might try and do it with the existing withStyles just to see what I come up with. |
@Sunderous The issue you linked is half a year old and closed. 🤔 Don't worry though, I will update the styles if you don't have the time. Your PR is a great start, I'll merge it and start building upon it. 👍 |
GitHub doesn't show it, but I merged this PR manually into the |
Any chance to push it to NPM? :) |
@jaulz The port is not finished, yet. 😕 I'll continue as soon as I have time. |
any updates? would love to use this. |
Greetings. This is my first time submitting a PR, so if I botched anything just let me know and I'll clean it up.
I was thinking of using this with a project, and wanted to use the new material UI, so I took a stab at updating it.
Browser still throws some warning from the material-ui-dots package, but looks like it functions properly in desktop, mobile, and mobile-landscape modes.