Skip to content
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

UI tweaks on mention-unsubscribed warning #4219

Merged
merged 3 commits into from
Aug 11, 2020
Merged

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Aug 6, 2020

This follows up on some of the UI points I noted in this comment before merging #4101: #4101 (comment)

Specifically, going through that comment in order:

  • button shape/size in visual appearance -- done here
  • button touch target to 48px (extending beyond visual button) -- still TODO
  • no border -- done here
  • background color -- still TODO
  • text/background color contrast, for legibility -- done in Warn on @-mentioning someone who won't see it because not subscribed #4101 before merge
  • cut animation -- done here
  • use a nice slide-in animation instead -- still TODO

@agrawal-d
Copy link
Member

Regarding the border, its present to separate multiple warnings.

@gnprice
Copy link
Member Author

gnprice commented Aug 7, 2020

Regarding the border, its present to separate multiple warnings.

Yeah. As I say in the commit message:

When there are several of these banners, this means there won't be a
dividing line between them.  I tried that out and it looked fine.

Here's a screenshot:
Screenshot_1596763759

When there are several banners, the effect is a lot like a Material list. A list like this one typically won't have dividers between the elements:

Place a divider between rows with lots of content, such as those with three-line lists.

Copy link
Member

@agrawal-d agrawal-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Aug 7, 2020

LGTM!

  • button touch target to 48px (extending beyond visual button) -- still TODO

For this, we'll likely want to use the hitSlop prop of the component that gets the onPress prop. We may have to adjust the dimensions of its parent, too:

The touch area never extends past the parent view bounds and the Z-index of sibling views always takes precedence if a touch hits two overlapping views.

…ing.

As it was, the button had conspicuously tall vertical padding, and
very little horizontal padding.
This border feels pretty heavy, and unbalanced because there isn't
a similar border at the bottom.

The distinct background color makes a nice sharp contrast against
the message list all by itself.

When there are several of these banners, this means there won't be a
dividing line between them.  I tried that out and it looked fine.
This animation looks kind of odd, and inappropriate for the entrance
of a banner.  See description here:
  zulip#4101 (comment)

Ideally we'd have an appropriate animation, like the banner sliding
in from the bottom.  But no animation at all actually looks pretty OK
to me here, and definitely better than this odd animation.

(We use the same odd animation for the "N unreads" banner, and it's
equally inappropriate there.  Likely we should just drop it there, too.)
@gnprice gnprice merged commit 7a23ac7 into zulip:master Aug 11, 2020
@gnprice gnprice deleted the pr-warning-ui branch August 11, 2020 04:46
@gnprice gnprice added the a-compose/send Compose box, autocomplete, camera/upload, outbox, sending label Aug 11, 2020
@gnprice
Copy link
Member Author

gnprice commented Aug 11, 2020

Merged! Thanks for the reviews.

I've filed #4224 for the remaining followups here. Including the mention of hitSlop -- that sounds very helpful! I hadn't known about that one and had imagined doing it by having ZulipButton contain some padding inside the touchable while outside the part that looks like a button. I think that could work, but this sounds potentially simpler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-compose/send Compose box, autocomplete, camera/upload, outbox, sending
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants