-
Notifications
You must be signed in to change notification settings - Fork 19
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
dependencies: Updates semantic-ui-react to latest #321
Conversation
@@ -0,0 +1,11 @@ | |||
import { createMedia } from '@artsy/fresnel'; | |||
|
|||
export const AppMedia = createMedia({ |
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.
Nice! Was this suggested by Semantinc-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.
Yes, there is a "migration" guide -> Semantic-Org/Semantic-UI-React#4008
@@ -70,7 +70,7 @@ export default class RelationModal extends Component { | |||
size="large" | |||
closeIcon | |||
trigger={ | |||
<> | |||
<div> |
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 div
needed for spacing?
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.
@jrcastro2 we might have to do the same in |
Yes, there are a couple of responsive elements in |
LGTM! did you check if the modals are working correctly ? |
Yes, they work correctly, we usually use the state of the component to store the open value of the modal so these breaking changes do not affect us. |
Updates SUI to the latest.
Breaking changes:
Responsive
element is removed.Followed the migration advice -> Semantic-Org/Semantic-UI-React#4008