compose: Use "info" intent, not "danger", for cannot-post-here banners #1871
Conversation
chrisbobbe
force-pushed
the
pr-cannot-post-banner-intent
branch
from
32f405e to
5106d65
Compare
|
Hm, did we get feedback on this? We do use red in the web app, though that's when the compose box is open. 
|
|
I guess the difference here is that this notice would always be visible in a channel you can't post in? Blue/info does seem reasonable in that case. |
|
Yeah, it's a non-dismissable banner that we show when we're not showing the compose box, because composing isn't allowed. I think "info" is appropriate because the user didn't do anything wrong and there wasn't any action that failed: they just want to catch up on |
rajveermalviya
left a comment
rajveermalviya
left a comment
There was a problem hiding this comment.
Thanks @chrisbobbe! LGTM, moving over to Greg's review.
rajveermalviya
added
integration review
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks for fixing this! A couple of comments below.
| return _ErrorBanner(getLabel: (zulipLocalizations) => | ||
| zulipLocalizations.errorBannerCannotPostInChannelLabel); | ||
| return _Banner( | ||
| intent: _BannerIntent.info, | ||
| label: zulipLocalizations.errorBannerCannotPostInChannelLabel); |
There was a problem hiding this comment.
Neat, I like that the label is now found in a more straightforward way.
lib/widgets/compose_box.dart
Outdated
| Widget build(BuildContext context) { | ||
| // (A BuildContext that's expected to remain mounted until the whole page | ||
| // disappears, which may be long after the banner disappears.) | ||
| final pageContext = PageRoot.contextOf(context); |
There was a problem hiding this comment.
Hmm, this seems odd, though. If the banner disappears, this widget will get unmounted. In that case what's going to end up subsequently using this pageContext value?
It looks to me like both the references below could just use context directly, and that'd be the normal Flutter thing to do.
… Aha, down at the bottom of the _handleTapSave implementation, it does actually want a longer-lasting context. (It awaits a request, and then might end up trying to display an error.) I think that's something that method should take care of, though.
There was a problem hiding this comment.
Probably cleanest to have a prep commit before this make the change to have buildTrailing take the local context, and _handleTapSave take care of getting the page context.
chrisbobbe
force-pushed
the
pr-cannot-post-banner-intent
branch
from
5106d65 to
bf0b314
Compare
|
Thanks for the reviews! Revision pushed. |
As we do for e.g. ZulipWebUiKitButton.
Instead of as a base class with subclasses _ErrorBanner and _EditMessageBanner.
|
Thanks! Looks good; merging. |
gnprice
force-pushed
the
pr-cannot-post-banner-intent
branch
from
bf0b314 to
38a2c63
Compare
4 participants
Fixes #1870.