-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[Doc] Convert the Snackbar to the new format #2562
[Doc] Convert the Snackbar to the new format #2562
Conversation
@oliviertassinari That would be great 👍 👍 a simple |
@@ -208,7 +209,7 @@ export default { | |||
borderColor: rawTheme.palette.borderColor, | |||
}, | |||
isRtl: false, | |||
zIndex: rawTheme.zIndex, | |||
zIndex: zIndex, |
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.
The idea of the rawTheme is to build the muiTheme.
So I think that the zIndex has nothing to do inside the rawTheme, as the isRtl.
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.
Yeah good call, I agree. 👍
@subjectix @newoga I'm done with this one. Could you review 😬? Thanks |
@@ -1,4 +1,5 @@ | |||
export default { | |||
snackbar: 900, |
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.
I think snack bar should be even higher than popover, don't you think? I'd say 2900
, must still be less than toolbar.
Wouldn't you agree?
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.
I had no idea about the right value. I'm gonna follow you 👍
Address those and merge as you wish. this is awesome 👍 👍 |
return { | ||
open: this.props.openOnMount, | ||
open: open, |
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.
might be able to make open
a const
or directly assign it to this object like
{
open: this.props.open || this.props.openOnMount,
}
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.
that won't check explicitly for null, it will coerce it to bool.
maybe:
const {
open,
openOnMount,
message,
action,
} = this.props;
return {
action,
message,
open: open !== null ? openOnMount : open,
muiTheme: this.context.muiTheme ? this.context.muiTheme : ThemeManager.getMuiTheme(DefaultRawTheme),
};
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.
I wanted to avoid the edge case where open and openOnMount are both set. I know, that's not common.
At least this open variable will be removed with v0.15
@oliviertassinari I gave it a quick look on github and it looks good to me. I definitely prefer it controlled and I like the new deprecatedProp utility function and that it appears it's tied up with the doc generation. 👍 👍 |
this._timerId = undefined; | ||
} | ||
|
||
componentWillMount() { |
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.
Oops it's componentWillUnMount
here
@oliviertassinari Awesome 👍 👍 feel free to merge this 👍 👍 |
[Doc] Convert the Snackbar to the new format
Thanks guys 🎉 |
Awesome 🎉 🎉 |
I think that we should make the
Snackbar
controlled as theDialog
.@subjectix Do you when me to do it in this PR?
Fix some point of #2572.