-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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][Core] Remove style-resizable mixin #2934
Conversation
The styles-resizable mixin is only used in one component: `snackbar.jsx`. It's used to see if the window.innerWidth is less than 768. This commit makes snackbar do a manual check against window to remove the mixin. Signed-off-by: Neil Gabbadon <[email protected]>
@@ -236,7 +234,7 @@ const Snackbar = React.createClass({ | |||
actionColor, | |||
} = this.constructor.getRelevantContextKeys(this.state.muiTheme); | |||
|
|||
const isSmall = this.state.deviceSize === this.constructor.Sizes.SMALL; | |||
const isSmall = window.innerWidth < 768; |
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 if the screen is rezised?
The component won't rerender 😁
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.
True, on one hand I figured this shouldn't matter to much because:
- Snackbars are generally temporary
- The resolution of small devices typically does not change
Though on second thought, there are some other considerations like the width as a result from a orientation change.
#2861 is using this mixin 😁. |
@oliviertassinari Yikes! Thanks for pointing that out. Mm...okay. Maybe we're going to have to come up with a closer to permanent solution before dropping this one. 😞 |
@oliviertassinari This approach isn't going to work and (I think) you will be taking this on soon. Either way, we can open up a new PR that remove this mixin. I'm closing this PR now. |
The
styles-resizable mixin
is only used in one component:snackbar.jsx
. It's used to see if thewindow.innerWidth
is less than 768. This commit makes snackbar do a manual check against window to remove the mixin.Let me know if you think we should deprecate instead of remove. I do think we need to revisit responsiveness but I don't think this mixin got us too far since it was only used in one component and the breakpoints used in the mixin (992 and 768) do not follow the spec.
I think we could probably create an issue as a reminder than there's a manual check being done for snackbar. Maybe we should create a Responsive issue label as well?