-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[v4] Switch react-transition-group versions #1759
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
@@ -0,0 +1,21 @@ | |||
declare module '@material-ui/react-transition-group' { |
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.
Why did we need to declare this module? Is it because the fork didn't include the types?
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.
Every package we consume needs types, however, most don't include them in the package. Polaris does include types, which is why we don't have a @types/shopify__polaris-react
. react-transition-group
does have a types package, which we're still consuming and I'm using to create type definitions for @material-ui/react-transition-group
which doesn't have a @types
package
@@ -124,6 +124,10 @@ $bulk-actions-offset-slide-in-start: rem(-40px); | |||
} | |||
} | |||
|
|||
.CheckableContainer { | |||
flex: 1 1 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.
Why was this needed? I see.
@@ -151,15 +151,13 @@ export function polarisContextReactWrapper<P, S>( | |||
} | |||
|
|||
return ( | |||
<React.StrictMode> |
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.
Why did you remove Strictmode, wasn't this the reason for this PR?
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.
Enzyme still uses it 😢 Although @shopify/react-testing
and storybook don't 😄
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.
WHY are these changes introduced?
Some of our dependencies use
findDOMNode
which is a strict mode nono 😭findDOMNode
findDOMNode
WHAT is this pull request doing?
findDOMNode
How to 🎩
Check out animations in the deployment (Frame, Toast, Modal, Dialog, ReosurceList, and Sheet)
Notes
CSSTransition
fromPopoverOverlay
react-transition-group
to suggest an alternative tofindDOMNode
but I'm not sure they'll accept the change since it's an api change and this doesn't seem like a priority