Tweak UI: Change some parts of UI for mention-unsubscribed warning. #4488

Gautam-Arora24 · 2021-02-19T08:30:17Z

This PR is intended to solve left-out parts of issue #4224.

This is how it looks

Screen.Recording.2021-04-10.at.8.49.00.PM.mov

chrisbobbe

Thanks, @Gautam-Arora24! See a few comments below.

chrisbobbe · 2021-03-16T19:47:24Z

src/common/ZulipButton.js

@@ -103,6 +104,7 @@ type Props = $ReadOnly<{|
  text: LocalizableText,
  secondary: boolean,
  onPress: () => void | Promise<void>,
+  hitSlop?: EdgeInsetsProp, // The EdgeInsets class specifies offsets in terms of visual edges, left, top, right, and bottom


ZulipButton's interface here is to accept a prop named hitSlop that works just like View's hitSlop prop, right? In particular, ZulipButton just takes hitSlop and passes it directly to a View.

I think it'd make a bit more sense to make that more explicit in this type, sort of like how we do with style. If it's quite clear that a prop like hitSlop is used just like View's hitSlop prop, then we don't need a code comment like this; people will naturally check the upstream doc for what it means. (And then, if the doc is unclear or misleading, we can add our own comments about that. But I don't think that's really the case here.)

Specifically, I'm thinking about something like this:

+ hitSlop?: $PropertyType<ElementConfig<typeof View>, 'hitSlop'>,

That's a way to say "hitSlop, when present, should have the same type as View's hitSlop prop". 🙂

chrisbobbe · 2021-03-16T19:50:46Z

src/message/MentionedUserNotSubscribed.js

@@ -54,6 +54,12 @@ const styles = createStyleSheet({
  buttonText: {
    color: 'black',
  },
+  hitSlop: {


nit: This doesn't belong in styles; it isn't used for a View's style prop, but rather a View's hitSlop prop.

chrisbobbe · 2021-03-16T19:53:29Z

src/message/MentionedUserNotSubscribed.js

+    top: 6,
+    right: 0,
+    bottom: 6,
+    left: 0,


How did you calculate what numbers to use here? 🙂

I see the commit message says the touch target now extends to 48px after setting the top and bottom hitSlop to 6px. But let's describe the starting point, so we can be sure 6px is right. 🙂 Let's also include a link to the doc that gives the 48 x 48 px guideline.

Here is the link to the docs that suggests 48 x 48 px guideline -> https://support.google.com/accessibility/android/answer/7101858?hl=en

chrisbobbe · 2021-03-16T20:10:19Z

src/message/MentionedUserNotSubscribed.js

@@ -67,7 +67,12 @@ class MentionedUserNotSubscribed extends PureComponent<Props> {
    const { user, onDismiss } = this.props;
    onDismiss(user);
  };
-
+  componentDidMount() {
+    LayoutAnimation.configureNext({ ...LayoutAnimation.Presets.easeInEaseOut, duration: 100 });


I like that these slide in and out now! I see something a bit odd, though—it looks like the warnings are sliding between the compose box's background and the compose box's inputs, in the z-direction. Can we fix that? 🙂

I think it might look better if it slid in and out from under the compose box entirely, i.e., not above its background.

Ah, also—the WebView jumps up and down a bit when these warnings enter and exit. Can we fix that? I think it'd probably be OK here to overlay the warnings above the WebView in the z-direction, instead of stacking them in the y-direction. Unlike the unread banner—see some recent discussion on that—these warnings can always be dismissed to see what's under them, without changing the state of the message list.

Here's a video with the animation slowed down so these things are easier to see:

mention-warning-animation.mp4

Done :) Now the Webview doesn't jump up and down and the warning overlay in the z-direction.

Gautam-Arora24 · 2021-04-11T03:07:13Z

@chrisbobbe This PR is ready for a re-review. Kindly have a look 🙂

Use a nice animation for this component. Use LayoutAnimation API to animate the component. Make use of componentDidMount and componentWillMount to make the component animate in both mount and unmount state. Make use of absolute positioning to make sure that the warning message does not move the Webview up and down. Also, now the warning message overlay on existing message in the z-direction. Discussion of this in CZO [1] [1] https://chat.zulip.org/#narrow/stream/48-mobile/topic/UI.20tweaks Fixes part of zulip#4224

Made the use of hitSlop prop and passed it from MentionedUserNotSubscribed to ZulipButton. Touch target now extends to 48px by specifying top and bottom properties of hitSlop prop to 6px each. Since the height of the button is 32, so adding the hitSlop to 6 at both top and bottom sums it up to 48. Link to the doc - https://support.google.com/accessibility/android/answer/7101858?hl=en Discussion of this in CZO [1] [1] https://chat.zulip.org/#narrow/stream/48-mobile/topic/UI.20tweaks Fixes part of zulip#4224

chrisbobbe reviewed Mar 16, 2021

View reviewed changes

Gautam-Arora24 force-pushed the UI_improvement branch 4 times, most recently from 7829567 to b903ada Compare April 10, 2021 15:34

Gautam-Arora24 requested a review from chrisbobbe April 10, 2021 15:44

Gautam-Arora24 force-pushed the UI_improvement branch from b903ada to 54c3d4b Compare April 11, 2021 03:10

Gautam-Arora24 force-pushed the UI_improvement branch from 54c3d4b to 1501c6b Compare April 30, 2021 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak UI: Change some parts of UI for mention-unsubscribed warning. #4488

Tweak UI: Change some parts of UI for mention-unsubscribed warning. #4488

Gautam-Arora24 commented Feb 19, 2021 •

edited

Loading

chrisbobbe left a comment

chrisbobbe Mar 16, 2021

Gautam-Arora24 Apr 10, 2021

chrisbobbe Mar 16, 2021

Gautam-Arora24 Apr 10, 2021

chrisbobbe Mar 16, 2021

chrisbobbe Mar 16, 2021

Gautam-Arora24 Apr 10, 2021

chrisbobbe Mar 16, 2021 •

edited

Loading

Gautam-Arora24 Apr 10, 2021

Gautam-Arora24 commented Apr 11, 2021

Tweak UI: Change some parts of UI for mention-unsubscribed warning. #4488

Are you sure you want to change the base?

Tweak UI: Change some parts of UI for mention-unsubscribed warning. #4488

Conversation

Gautam-Arora24 commented Feb 19, 2021 • edited Loading

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gautam-Arora24 commented Apr 11, 2021

Gautam-Arora24 commented Feb 19, 2021 •

edited

Loading

chrisbobbe Mar 16, 2021 •

edited

Loading