-
-
Notifications
You must be signed in to change notification settings - Fork 248
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
[WIP] Support material-ui 1.0.0 #427
Conversation
Codecov Report
@@ Coverage Diff @@
## master #427 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 157 157
Lines 1401 1404 +3
=====================================
+ Hits 1401 1404 +3
Continue to review full report at Codecov.
|
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.
Few things:
- (demo project) If you'll add a new
friend
field item, fill the first one and remove it, then theage
is still there. It's not the case in other themes or currentuniforms-material
. - There are few more ordering problems, so maybe let's skip them for now and let's add it to the ESLint. What do you think?
- I'm a bit concerned about all the new props being passed to some in-between components, like the
<FormControl />
or<List />
. It may be a problem to mantain all of them.
import AlarmIcon from '@material-ui/icons/Alarm'; | ||
import React from 'react'; | ||
import {mount} from 'enzyme'; | ||
import IconButton from '@material-ui/core/IconButton'; |
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.
Ordering.
import AlarmIcon from '@material-ui/icons/Alarm'; | ||
import React from 'react'; | ||
import {mount} from 'enzyme'; | ||
import IconButton from '@material-ui/core/IconButton'; |
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.
Ordering.
import {mount} from 'enzyme'; | ||
|
||
import SelectField from 'uniforms-material/SelectField'; | ||
import Select from '@material-ui/core/Select'; |
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.
Ordering.
"uniforms": "^1.24.3" | ||
}, | ||
"dependencies": { | ||
"babel-runtime": "^6.20.0", | ||
"prop-types": "^15.5.7" |
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.
Are these changes necessary? Also, babel-runtime
is not required anymore?
"react": "^16.0.0 || ^15.0.0 || ^0.14.0", | ||
"@material-ui/core": "^1.0.0", | ||
"@material-ui/icons": "1.0.0", | ||
"react": "^16.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.
Is this a @material-ui/core
requirement?
import joinName from 'uniforms/joinName'; | ||
import List from '@material-ui/core/List'; | ||
import ListSubheader from '@material-ui/core/ListSubheader'; | ||
import React from 'react'; |
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.
Ordering.
)} | ||
</List> | ||
<ListAddField name={`${name}.$`} initialCount={initialCount} /> | ||
</Fragment> |
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.
If @material-ui/core
doesn't require react@16
, I'd rather go with the <div />
and have more compatibility.
allowedValues, | ||
checkboxes, // eslint-disable-line no-unused-vars |
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.
What's the case here?
|
||
const Radio = ({ | ||
const Radio_ = ({ |
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.
If so, use the displayName
to let the field be called RadioField
.
|
||
import ListAddField from './ListAddField'; | ||
import ListItemField from './ListItemField'; | ||
|
||
const List = ({ | ||
actionsStyle, | ||
const List_ = ({ |
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.
If so, use the displayName
to let the field be called ListField
.
…icons, ensureValue: true for fields
…ames, changes for dependencies
So, we have it! I'll do a pre-release soon. |
This is follow up of #355
I still want to add support for
helperText
property.Tests need too be fixed.