-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Item: Update to v1 API #431
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
Conversation
@levithomason |
The deprecatedComponents export old names, but replace them with another component: -export const Items = deprecateComponent('Items', 'Use "Item.Items" instead.', Item.Items)
+export const Items = deprecateComponent('Items', 'Use "Item.Group" instead.', Item.Group) The last argument is the replacement component. In this case, it will now export |
Current coverage is 96.69% (diff: 100%)@@ master #431 diff @@
==========================================
Files 84 90 +6
Lines 1254 1300 +46
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 1206 1257 +51
+ Misses 48 43 -5
Partials 0 0
|
import Image from '../../elements/Image/Image' | ||
|
||
function ItemImage(props) { | ||
return <Image {...props} /> |
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.
@levithomason I have problem there.
- I think I need use
createImage
factory, but will it pass props? ItemImage
doesn't haveui
class by default. How we can deal with this?
@levithomason, finished ✌️ Can be merged after issue will be solved and code review 😺 |
Taking a look... |
I've added a <Image {...rest} as={ElementType} ui={false} /> This seems to solve the issue with the |
There are a couple warnings to solve, looks like we're spreading props we shouldn't be:
|
… into fix/item Conflicts: src/views/Item/ItemGroup.js src/views/Item/ItemImage.js
Seems, it fixed 👍 |
} = props | ||
|
||
const classes = cx( | ||
'ui', | ||
ui && 'ui', |
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.
Stupid question. Why you used there '&&' instead of useKeyOnly
? 🐰
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.
Stupid answer, whoops!
Will merge/release once tests pass |
Released in |
Finished
Update Item component to v1 API.