Put lightbox header/footer text and buttons in the safe area. #4442

chrisbobbe · 2021-01-28T23:49:50Z

This branched off of #4440 when the fix became a bit more complicated than what was mainly being done in that PR. 🙂

I've addressed some of Greg's points there:

It looks like this causes the header and footer to get separated from the top and bottom of the screen. That seems like an odd effect. Better would be for the background of the header and footer to extend all the way to the top and bottom, respectively.

Yeah, you're right. I'd hesitated to obscure more of the image, which is why I had just those bands, instead of extending the space through the whole top and bottom. But then, yeah—it doesn't really matter at all if we obscure that bit of the image, since the user can just tap, and the header and footer will go away completely. 🙂

Ah, but if I rotate the device into landscape mode, then the screenshot gets scaled down to fit -- it gets "pillarboxed". We should do the same, I think. It looks like we're instead doing some unhelpful cropping.

Yeah. Unfortunately, that doesn't get fixed in this PR; I haven't quite found the right way to do it. There's a resizeMode prop that I add a code comment on, saying that it basically doesn't work at all on iOS. It's not fixed in react-native-photo-view-ex, which you pointed out at #4217 (comment). I notice that if the device is already in landscape mode when you open the lightbox, it shows the whole image, as we'd want it to:

But then, from there, if you go into portrait mode, the image is incorrectly small:

I suspect the native component gets initialized with proper dimensions but doesn't bother to properly update its dimensions after that.

I tried a React key hack to unmount and remount the PhotoView component when the orientation changes. That worked for maybe 75% of rotations, but the rest of the time it wouldn't unmount-remount upon rotation, instead updating when I tapped the screen, despite the key actually changing. Not sure what's going on there. Also I'm pretty sure the unmount-remount means the image gets re-downloaded.

Anyway, even with the improper framing when you're in landscape mode, it is possible to scroll so that you can see all of the image; it just doesn't all show at once.

Hmm, does the status bar really appear in the lightbox?

Ah, I think it might only be showing up after a fast-refresh (from when save changes to a file), and those don't happen in production.

We should show the status bar -- the real live one from the device right now 🙂 -- when we're showing the header and footer. I think we currently don't, and don't in your screenshots from after this PR.

We should hide it when we're hiding the header and footer. I think we currently get this right (and from the code in the PR, I think it doesn't change that.)

I'll tackle this after some more work in the clean-up-navigation PRs. Currently, ZulipStatusBar is improperly doing the job of protecting content from the unsafe areas, with a sized View, so inserting it or removing it somewhere will unhelpfully alter the layout. I've got a draft that takes that View out of ZulipStatusBar.

Here are the screenshots after the changes in this PR:

I also tested on Android, and my iPhone 6s, and all seems fine.

Fixes: #4267

gnprice

Thanks @chrisbobbe ! Comments below.

I'll tackle [the status bar] after some more work in the clean-up-navigation PRs. Currently, ZulipStatusBar is improperly doing the job of protecting content from the unsafe areas, with a sized View, so inserting it or removing it somewhere will unhelpfully alter the layout. I've got a draft that takes that View out of ZulipStatusBar.

Sounds like a good plan to me!

gnprice · 2021-01-29T00:17:15Z

src/ZulipMobile.js

+    // In the future, layout animation will be enabled by default, and
+    // this method will be removed:
+    // https://github.com/facebook/react-native/blob/v0.63.4/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java#L741-L755.
+    throw new Error('Expected `UIManager.setLayoutAnimationEnabledExperimental` to exist.');


Hmm. That is what the doc comment says, but I'm a bit skeptical. RN does not have a strong record of following through on the things that their docs say are totally going to happen any time now 😛

This particular prediction of the future looks like it was added in 2015, in facebook/react-native@593a45e.

There was a commit in 2017-03, facebook/react-native@abc483a, that said

BREAKING - Remove LayoutAnimation experimental flag on Android Summary: I don't remember exactly where we talked about this but LayoutAnimation on Android is pretty stable now so there's no reason to keep it behind an experimental flag anymore. The only part that is not really stable is delete animations, so what I did is remove the default delete animation that we provide in presets.

but it promptly got reverted.

Another attempt in 2019-04 got promptly reverted too, by facebook/react-native@d9a8222:

Back out "[react-native] Remove experimental gating for LayoutAnimation on Android" Summary: We've identified a couple of remaining issues that need to be re-tested before we can ship this more broadly.

I guess what that all means for us is:

In this comment, say "may be enabled" rather than "will", to not reflect too much credence in that upstream comment's promise. 🙂

We should be on the lookout for this to cause issues on Android. It sounds like it probably works fine for a lot of use cases, but it's not clear where the gaps are.

gnprice · 2021-01-29T00:19:28Z

src/ZulipMobile.js

+    // In the future, layout animation will be enabled by default, and
+    // this method will be removed:
+    // https://github.com/facebook/react-native/blob/v0.63.4/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java#L741-L755.
+    throw new Error('Expected `UIManager.setLayoutAnimationEnabledExperimental` to exist.');


Separately: instead of checking for existence and throwing an exception if missing, how about simply charging ahead and invoking the method?

If it's missing, that'll cause an exception; and it doesn't seem like this one adds a lot relative to that. This is all premised on an expectation that if/when we upgrade to an RN version where this is gone, we'll hit this failure in dev first thing and immediately fix it; if this were something where we couldn't expect that, the exception wouldn't be a good solution either.

gnprice · 2021-01-29T00:36:16Z

src/lightbox/LightboxFooter.js

@@ -22,7 +21,7 @@ const styles = createStyleSheet({
    alignSelf: 'center',
  },
  icon: {
-    fontSize: 28,
+    fontSize: 36,


Hmm, that's a pretty big icon.

In the bottom tabs on the main screen, we have icons of size 24. That seems to be the standard size for most icon buttons in Material Design; see for example https://material.io/components/bottom-navigation#specs , or https://material.io/components/app-bars-bottom#specs for the three-dots "overflow menu" icon. How does this look if we make it size 24?

(I agree it'd be good to make both icons the same size.)

gnprice · 2021-01-29T01:09:18Z

src/lightbox/Lightbox.js

@@ -80,7 +80,7 @@ export default function Lightbox(props: Props) {
          styles.overlay,
          styles.header,
          { width: windowWidth },
-          movement === 'out' ? { top: 0 } : { bottom: windowHeight },


Oh wow, 'out' meant they were visible? That sure was confusing.

gnprice · 2021-01-29T01:14:44Z

src/lightbox/Lightbox.js

+        showsHorizontalScrollIndicator={false}
+        showsVerticalScrollIndicator={false}


Lightbox: Remove scrollbars when panning and zooming on iOS. I don't think these really add anything.

Agreed.

From trying the current beta on Android, I think the "on iOS" means that currently we have scrollbars on iOS but not Android, and after this change we don't have them on either platform. Does that agree with what you have in mind? Would be good to make explicit -- otherwise it sounds like perhaps we still have them but only on Android.

Right -- these are iOS-only props. I'll add a code comment.

gnprice · 2021-01-29T01:21:43Z

src/lightbox/Lightbox.js

+        // Doesn't seem to do anything on iOS:
+        //   https://github.com/alwx/react-native-photo-view/issues/62
+        //   https://github.com/alwx/react-native-photo-view/issues/98
+        // TODO: Figure out how to make it work.
        resizeMode="contain"


Huh, I see! Indeed, when I try on Android (with the current beta) I get exactly the "pillarboxed" behavior that I'd hope for. It's just on iOS that I don't, and the image stays full size.

Hmm. I've been doing this testing mainly with an image that's a screenshot from a similar phone, so its natural size is the screen size. What if the image's natural size is much larger, or smaller? Does this issue in PhotoView mean that on iOS it'll stay too large, or too small?

Here are some screenshots from the iOS simulator, for an extremely small image, and an extremely large image. I've included screenshots of portrait and landscape mode, but it's important to note that, in both cases, I started out in portrait mode, since that seems to make a difference.

I think the native component showing the image sets its dimensions when it first initializes, but it doesn't update them at all afterward.

I think the native component showing the image sets its dimensions when it first initializes, but it doesn't update them at all afterward.

Cool, yeah, that seems to be what's happening. So it appropriately scales the image at the start, but doesn't update that when the component gets resized.

gnprice · 2021-01-29T01:28:51Z

src/lightbox/Lightbox.js

+  // Since we're using `Dimensions.get` (below), we'll want a rerender
+  // when the orientation changes. No need to store the value.
+  useSelector(state => getSession(state).orientation);


Oh, I think I see this as a bug in the existing implementation too!

Start in portrait mode, open the lightbox, and then rotate to landscape. The image stays limited to a pillarbox, even if the image's natural aspect ratio is wider. And the lightbox header and footer stay narrow, too:

I'd noticed that a few minutes ago, and was going to mention it as another small design issue that'd be good to fix while we're working on this -- the header and footer should go the whole width of the screen. Then I realized that it's caused by the bug this change fixes 🙂

Not sure if I'd ever spotted that before; if I had, I must have put it down to a suboptimal design choice then too.

Conversely, if you start in landscape and rotate to portrait, everything stays wide, and the header and footer are unusable because the controls are off-screen. That must be this same bug. (And, even before seeing this code, would be a signal that it's an outright bug and not a dubious design choice.)

gnprice · 2021-01-29T01:37:30Z

src/lightbox/Lightbox.js

@@ -49,6 +49,7 @@ export default function Lightbox(props: Props) {

  // Pulled out here just because this function is used twice.
  const handleImagePress = useCallback(() => {
+    LayoutAnimation.configureNext(LayoutAnimation.create(100, 'easeInEaseOut', 'opacity'));


It'd be good to be clear that this doesn't just reimplement the old animation -- it replaces it with a different animation.

In particular the old animation had the header and footer slide in from the top and bottom, while this one has them just fade in.

It's a shame that (AFAICT) this LayoutAnimation API doesn't seem to support translation at all -- I think it would not be possible to implement an animation like the old one using this API.

I kind of like the slide-in/out animation. But it looks like both Google's and Apple's "Photos" apps have the top and bottom bars fade in and out, with no sliding; so even though I'm not sure I like that quite as much, it's clearly a perfectly fine way to do it.

... Hmm, and now I look back at your (animated) screenshots, and it looks like it still is doing slide-in/out! I like the effect -- but I sure am confused about what that 'opacity' argument is doing. According to the doc:
https://reactnative.dev/docs/layoutanimation
it should be "the layout property to animate"; but in your screencaps it looks like top and bottom, or really the overall position that they together work out to, is animating, and opacity isn't at all.

Huh, yeah, odd—I also don't see anything happening with the opacity; it's just sliding in and out. I took Presets.easeInEaseOut and just adjusted the duration from 300 to 100, since it seemed kind of long. (The new revision will be clearer about this.)

Not sure what Presets.easeInEaseOut has in mind for 'opacity'.

We'll use this soon to redo the header and footer animations in `Lightbox`. Greg points out [1] that, while a code comment in React Native says that activating this 'experimental' code will one day be unnecessary, we should be skeptical that that'll actually happen, given React Native's record. :) [1] zulip#4442 (comment)

All of these values were off by the amount of `windowHeight - 44` from sensible values. At least they were off together, so, there was no bug. :)

This way, we can better compare the logic for the header with the footer.

We should have been doing this already [1]. Without it, there would be a noticeable hiccup when you change the orientation, following the upcoming redo of the slide-in slide-out logic. [1] zulip#4442 (comment)

We'll re-implement it in the next commit, but this'll give some space to this change and that change, so they're both easier to follow.

The previous way we did this animation depended on Lightbox knowing (or guessing! NAVBAR_SIZE was a wrong guess) the heights of LightboxHeader and LightboxFooter [1]. This wasn't ideal, partly because it meant coordinating between `Lightbox` and those components, but also because the most reasonable, bug-free way to know the height of something is to set an explicit height, and that would mean limiting our options for being faithful to the user's system font-size setting. I also want to bring the footer into a more consistent appearance with the header, and as part of that I want to remove its explicit height. The `LayoutAnimation` API [2] is described in the "Animations" doc [3] as being powerful but not as granular as the plain Animated API [4]; it lets you animate "layout updates without bothering to measure or calculate specific properties" -- which is just what we're looking for. The new animation is meant to roughly match the old; the header and footer still slide in and out, though it's probable that the position vs. time graph looks slightly different now than before. See also discussion [5]. [1] I tried using values like "0%" and "100%" for `from` and `to` (`from` and `to` being the values that get plugged into `transform.translateY`), which would mean we wouldn't have to know the heights. But using percentages isn't supported yet; see facebook/react-native#13107. [2] https://reactnative.dev/docs/layoutanimation [3] https://reactnative.dev/docs/animations#layoutanimation-api [4] https://reactnative.dev/docs/animated [5] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60View.60's.20.60onLayout.60.20prop/near/1110130

As Greg points out [1], 24px seems to be the standard size for most icon buttons in Material Design [2] [3] [4]. [1] zulip#4442 (comment) [2] https://material.io/components/bottom-navigation#specs [3] https://material.io/components/app-bars-bottom#specs [4] https://material.io/design/iconography/system-icons.html#grid-and-keyline-shapes

chrisbobbe · 2021-01-29T20:44:49Z

Thanks for the review, revision pushed! I made another change; I've started using RN v0.63's new Pressable API. I also filed oblador/react-native-vector-icons#1301.

I've had some luck viewing touchable areas by doing the following:

shake
"Show Inspector"
"Touchables" (then the app annoyingly reloads, taking me away from the lightbox)
shake
"Hide Inspector"
go back to the lightbox

In this screenshot, it's very faint, but there's a red dotted-line square around the "x" and the three-dots button, showing the area of each touch target, from the tip of this PR:

gnprice

Thanks for the revision! Some comments below on the new Pressable commit; otherwise all LGTM.

gnprice · 2021-01-29T21:33:02Z

src/lightbox/LightboxHeader.js

@@ -73,9 +73,11 @@ export default class LightboxHeader extends PureComponent<Props> {
              {subheader}
            </Text>
          </View>
-          <Touchable style={styles.rightIconTouchTarget} onPress={onPressBack}>


On iOS, the "x" button in the header was putting a brand-colored rectangle around the icon when it was pressed, which seemed jarring and artificial to me.

Hmm, yeah.

I actually reproduce that on Android too, now that I look closely. I think it's less visible there, because the animation that takes us back to the message list is quicker to get it out of view. On Android we get the standard Android "fade down" animation for returning to a previous screen, and on iOS we get the standard iOS "slide out right" animation, and I guess the latter has a slower start.

gnprice · 2021-01-29T21:34:33Z

src/lightbox/LightboxFooter.js

@@ -39,7 +41,11 @@ export default class LightboxFooter extends PureComponent<Props> {
      <SafeAreaView mode="padding" edges={['right', 'bottom', 'left']}>
        <View style={[styles.wrapper, style]}>
          <Text style={styles.text}>{displayMessage}</Text>
-          <Icon style={styles.icon} color="white" name="more-vertical" onPress={onOptionsPress} />
+          <Pressable style={styles.iconTouchTarget} onPress={onOptionsPress} hitSlop={10}>


Neither button was giving the touch target the minimum 48 x 48 px size recommended by Material Design [1]; so, do that with the `hitSlop` prop [2]. Passing one number like this adds that amount of additional distance outside the element in which a press can be detected. So, with a font size of 28, the new width and height of the pressable area are approx. 10 + 28 + 10 = 48.

One thing I see in that handy Pressable doc is this:

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.

So this hitSlop will only take effect to the extent that there's that much room around this view within its parent view. Is that the case here? We might need to arrange some margin here, or padding in the parent, to make sure of that.

Ah, I think your screenshot in the comment above may answer this question. It looks like we're probably in good shape except for the top side of the three-dots button.

It'd be good to add some padding or margin to give that target its full size, and perhaps also to structure things so that it's clear they both do. Looking at the header code, I think that one is getting enough room only because the avatar holds up the header's height. Perhaps add a margin to each Pressable's style, equal to its hitSlop?

Perhaps add a margin to each Pressable's style, equal to its hitSlop?

Hmm, this feels like too much vertical space. The margin I've added gets added to the paddingVertical: 8 in styles.wrapper. It feels like there should be a way to ignore that padding, but I haven't found it yet.

(I suppose the same is happening with the horizontal padding, too.)

Ah, indeed.

I think it'd be fine to add margin that only supplements the parent's padding. We want 12 total space on each side (as of the end of this branch, after shrinking the icons to 24); so where the parent's padding is 16 that's enough by itself, but where it's 8 we want a margin of 4.

gnprice · 2021-01-29T21:45:27Z

src/lightbox/LightboxFooter.js

@@ -20,9 +20,11 @@ const styles = createStyleSheet({
    fontSize: 14,
    alignSelf: 'center',
  },
+  iconTouchTarget: {
+    alignSelf: 'center',
+  },
  icon: {
    fontSize: 28,


detected. So, with a font size of 28, the new width and height of the pressable area are approx. 10 + 28 + 10 = 48.

It is kind of odd that we're specifying this with something called fontSize. There's not really a font involved here. And it makes me wonder if this size is getting scaled by the user's font-size setting -- which would be the wrong behavior, because that setting should only affect actual text that the user may be trying to read as text.

OK, a quick test finds that it isn't (as of v27.158, current beta), neither on Android nor iOS. If I crank the system font-size setting way up, the text in the header and footer get bigger (as they should), but the icons stay the same size (as they should.)

Still, it'd be good if we can avoid that confusion. And as your "approx." suggests, it's not entirely clear that fontSize of 28 means a size of 28px.

It looks like in other places where we create an icon, we tend to use its own size prop, like size={24}, instead of using the style prop. If we do that here (with the same number as for fontSize), does that keep the behavior unchanged?

We've just replaced our only use of this with something that has a better API; see discussion [1]. Also, its interface seemed a little confused -- if it was meant to "slide" something, I wouldn't expect callers themselves to have to specify, e.g., `translateX` or `translateY`, over things that clearly don't make things slide, like `rotateX` or `skewX` [2]. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60View.60's.20.60onLayout.60.20prop/near/1110130 [2] https://reactnative.dev/docs/transforms

Including left and right safe areas, for landscape mode. Fixes: zulip#4267

…lts. `backgroundColor` and `borderBottomWidth` seem like common sense, but I didn't find specific documentation about their default values; `justifyContent` is documented [1] as defaulting to 'flex-start'. [1] https://reactnative.dev/docs/layout-props#justifycontent

This particular line wasn't well-explained when it was added in 058005c, and I don't notice any changes when I remove it. It's hard to imagine we'd want the children of this `styles.wrapper` view -- which includes the "X" button that should be fixed at the right side -- to wrap onto another line.

Now that we've trimmed away a few unnecessary properties in `styles.wrapper` in LightboxHeader, it's easier to see how it differs from the same set of styles in LightboxFooter. Staring at those differences, some of them don't make a lot of sense, and it seems like an opportunity to give the layout a more consistent feel. Make the header and the footer more consistent by - making the three-dots icon and the the "X" icon the same size - using the same horizontal padding, so the three-dots icon aligns with the "X" icon in the header - using the same vertical padding, instead of the arbitrary `height: 44`, for the vertical layout

I don't think these really add anything.

That is, center it in the content area of the header, not including the space in the top inset. Like what the three-dots icon in the footer does. The `alignSelf` property was effectively being ignored; now, it's not.

Greg points out [1] that it doesn't really make sense to set the size with `fontSize`. [1] zulip#4442 (comment)

The "x" button in the header was putting a brand-colored rectangle around the icon when it was pressed, which seems jarring and artificial. We often want this rectangle, in cases where its shape follows contours that are visible when the button is not touched (e.g., stream and topic items in the unreads list have their own clear rectangles that just become highlighted when you press them). But here, it's not following any contours like that; the rectangle has seemed kind of random and unnecessarily sharp. The three-dots button was giving touch feedback in a much nicer way: slightly dimming just the icon itself, and no rectangle around it. So, make that happen for both buttons. The dimming effect was small enough to be almost unnoticeable, so the new touch-feedback response (turning it from "white" to "gray") increases it a bit. Neither button was giving the touch target the minimum 48 x 48 px size recommended by Material Design [1]; so, do that with the `hitSlop` prop [2]. Passing one number like this adds that amount of additional distance outside the element in which a press can be detected. So, with a size of 28, the new width and height of the pressable area are 10 + 28 + 10 = 48. This is our first experimentation with the Pressable API [3], just added in React Native v63. [1] https://material.io/design/usability/accessibility.html#layout-and-typography [2] https://reactnative.dev/docs/pressable#hitslop; also available on `View` and other components. [3] https://reactnative.dev/docs/pressable

As Greg points out [1], 24px seems to be the standard size for most icon buttons in Material Design [2] [3] [4]. [1] zulip#4442 (comment) [2] https://material.io/components/bottom-navigation#specs [3] https://material.io/components/app-bars-bottom#specs [4] https://material.io/design/iconography/system-icons.html#grid-and-keyline-shapes

The extension of a view's touchable area caused by the `hitSlop` prop can be restricted by a few factors [1] [2]: """ 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. """ We don't want the buttons' parent view bounds to reduce the touchable area at all. So, to be sure they don't, grow the parent view bounds just enough. Horizontally, we're already in good shape and we don't need to do anything; the parent has a horizontal padding of 16, which keeps the touchable area away from the right edge of the screen. Vertically, the parent has 8 padding above and 8 padding below. Increase these to 12 (the value we've chosen for `hitSlop`) by giving the button an upper margin of 4 and a lower margin of 4. See discussion [3]. [1] https://reactnative.dev/docs/pressable#hitslop [2] https://reactnative.dev/docs/view#hitslop [3] zulip#4442 (comment)

As with the upgrade to v7, in 5ecf10e, only one breaking change was announced in 8.0.0 [1], and it's for something we don't use: - `Icon.TabBarItemIOS` convenience component removed as it was removed from React Native core. It also advertises better compatibility with Xcode 12. I hadn't run into any problems with Xcode 12, but still, this sounds helpful. So, might as well take the latest version. [1] https://github.com/oblador/react-native-vector-icons/releases/tag/v8.0.0

chrisbobbe · 2021-01-30T01:55:07Z

Thanks for the review! Revision pushed.

gnprice · 2021-01-30T02:56:10Z

Looks good! Merged.

We'll use this soon to redo the header and footer animations in `Lightbox`. Greg points out [1] that, while a code comment in React Native says that activating this 'experimental' code will one day be unnecessary, we should be skeptical that that'll actually happen, given React Native's record. :) [1] zulip#4442 (comment)

We should have been doing this already [1]. Without it, there would be a noticeable hiccup when you change the orientation, following the upcoming redo of the slide-in slide-out logic. [1] zulip#4442 (comment)

Greg points out [1] that it doesn't really make sense to set the size with `fontSize`. [1] zulip#4442 (comment)

As Greg points out [1], 24px seems to be the standard size for most icon buttons in Material Design [2] [3] [4]. [1] zulip#4442 (comment) [2] https://material.io/components/bottom-navigation#specs [3] https://material.io/components/app-bars-bottom#specs [4] https://material.io/design/iconography/system-icons.html#grid-and-keyline-shapes

The extension of a view's touchable area caused by the `hitSlop` prop can be restricted by a few factors [1] [2]: """ 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. """ We don't want the buttons' parent view bounds to reduce the touchable area at all. So, to be sure they don't, grow the parent view bounds just enough. Horizontally, we're already in good shape and we don't need to do anything; the parent has a horizontal padding of 16, which keeps the touchable area away from the right edge of the screen. Vertically, the parent has 8 padding above and 8 padding below. Increase these to 12 (the value we've chosen for `hitSlop`) by giving the button an upper margin of 4 and a lower margin of 4. See discussion [3]. [1] https://reactnative.dev/docs/pressable#hitslop [2] https://reactnative.dev/docs/view#hitslop [3] zulip#4442 (comment)

We'll use this soon to redo the header and footer animations in `Lightbox`. Greg points out [1] that, while a code comment in React Native says that activating this 'experimental' code will one day be unnecessary, we should be skeptical that that'll actually happen, given React Native's record. :) [1] zulip#4442 (comment)

We should have been doing this already [1]. Without it, there would be a noticeable hiccup when you change the orientation, following the upcoming redo of the slide-in slide-out logic. [1] zulip#4442 (comment)

Greg points out [1] that it doesn't really make sense to set the size with `fontSize`. [1] zulip#4442 (comment)

As Greg points out [1], 24px seems to be the standard size for most icon buttons in Material Design [2] [3] [4]. [1] zulip#4442 (comment) [2] https://material.io/components/bottom-navigation#specs [3] https://material.io/components/app-bars-bottom#specs [4] https://material.io/design/iconography/system-icons.html#grid-and-keyline-shapes

The extension of a view's touchable area caused by the `hitSlop` prop can be restricted by a few factors [1] [2]: """ 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. """ We don't want the buttons' parent view bounds to reduce the touchable area at all. So, to be sure they don't, grow the parent view bounds just enough. Horizontally, we're already in good shape and we don't need to do anything; the parent has a horizontal padding of 16, which keeps the touchable area away from the right edge of the screen. Vertically, the parent has 8 padding above and 8 padding below. Increase these to 12 (the value we've chosen for `hitSlop`) by giving the button an upper margin of 4 and a lower margin of 4. See discussion [3]. [1] https://reactnative.dev/docs/pressable#hitslop [2] https://reactnative.dev/docs/view#hitslop [3] zulip#4442 (comment)

Like we did in 2e5ff37. As mentioned there: """ As Greg points out [1], 24px seems to be the standard size for most icon buttons in Material Design [2] [3] [4]. [1] zulip#4442 (comment) [2] https://material.io/components/bottom-navigation#specs [3] https://material.io/components/app-bars-bottom#specs [4] https://material.io/design/iconography/system-icons.html#grid-and-keyline-shapes """

Like we did in 2e5ff37. As mentioned there: """ As Greg points out [1], 24px seems to be the standard size for most icon buttons in Material Design [2] [3] [4]. [1] #4442 (comment) [2] https://material.io/components/bottom-navigation#specs [3] https://material.io/components/app-bars-bottom#specs [4] https://material.io/design/iconography/system-icons.html#grid-and-keyline-shapes """

gnprice · 2021-07-13T05:13:24Z

(For cross-reference: this fixed part of #3066.)

chrisbobbe requested a review from gnprice January 28, 2021 23:49

chrisbobbe changed the title ~~Put lightbox header/footer content and controls in the safe area.~~ Put lightbox header/footer text and buttons in the safe area. Jan 29, 2021

chrisbobbe mentioned this pull request Jan 29, 2021

Clean up navigation logic (2/x). #4440

Merged

gnprice reviewed Jan 29, 2021

View reviewed changes

chrisbobbe added 7 commits January 29, 2021 10:32

lightbox footer [nfc]: Simplify some layout arithmetic.

f75e248

All of these values were off by the amount of `windowHeight - 44` from sensible values. At least they were off together, so, there was no bug. :)

Lightbox [nfc]: Be explicit, instead of depending on a default value.

4b3de0f

This way, we can better compare the logic for the header with the footer.

Lightbox [nfc]: Slightly rearrange some styles.

1042e35

Lightbox: Subscribe to updates in the orientation.

da74da5

We should have been doing this already [1]. Without it, there would be a noticeable hiccup when you change the orientation, following the upcoming redo of the slide-in slide-out logic. [1] zulip#4442 (comment)

Lightbox: Remove slide animation of header and footer.

cc0126d

We'll re-implement it in the next commit, but this'll give some space to this change and that change, so they're both easier to follow.

chrisbobbe force-pushed the pr-fix-lightbox branch from 3d97512 to ba9956c Compare January 29, 2021 20:39

gnprice reviewed Jan 29, 2021

View reviewed changes

chrisbobbe added 14 commits January 29, 2021 20:35

lightbox header/footer: Put content in safe areas.

e0784fe

Including left and right safe areas, for landscape mode. Fixes: zulip#4267

Lightbox [nfc]: Clarify header/footer state variable.

b66cda3

Lightbox [nfc]: Mark an annoying lack in the library.

c1b5ac8

Lightbox: Remove scrollbars when panning and zooming on iOS.

d6c3be2

I don't think these really add anything.

LightboxHeader: Center "x" button vertically.

5379c9c

That is, center it in the content area of the header, not including the space in the top inset. Like what the three-dots icon in the footer does. The `alignSelf` property was effectively being ignored; now, it's not.

lightbox header/footer: Use size prop.

9a502e8

Greg points out [1] that it doesn't really make sense to set the size with `fontSize`. [1] zulip#4442 (comment)

chrisbobbe force-pushed the pr-fix-lightbox branch from ba9956c to 4bc42a9 Compare January 30, 2021 01:55

gnprice merged commit 4bc42a9 into zulip:master Jan 30, 2021

chrisbobbe mentioned this pull request Jan 30, 2021

Clean up navigation logic (3/x). #4443

Merged

Gautam-Arora24 pushed a commit to Gautam-Arora24/zulip-mobile that referenced this pull request Feb 2, 2021

lightbox header/footer: Use size prop.

c50de27

Greg points out [1] that it doesn't really make sense to set the size with `fontSize`. [1] zulip#4442 (comment)

Gautam-Arora24 pushed a commit to Gautam-Arora24/zulip-mobile that referenced this pull request Feb 3, 2021

lightbox header/footer: Use size prop.

1bee499

Greg points out [1] that it doesn't really make sense to set the size with `fontSize`. [1] zulip#4442 (comment)

This was referenced Mar 13, 2021

"Unread messages" causes screen to jump when appearing/disappearing #4526

Open

Allow muting topic from within topic narrow #4532

Closed

gnprice mentioned this pull request Mar 18, 2021

ActionSheets: Refactor to tighten up types #4541

Merged

chrisbobbe deleted the pr-fix-lightbox branch November 4, 2021 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put lightbox header/footer text and buttons in the safe area. #4442

Put lightbox header/footer text and buttons in the safe area. #4442

chrisbobbe commented Jan 28, 2021 •

edited

Loading

gnprice left a comment

gnprice Jan 29, 2021

gnprice Jan 29, 2021

gnprice Jan 29, 2021

gnprice Jan 29, 2021

gnprice Jan 29, 2021

chrisbobbe Jan 29, 2021 •

edited

Loading

gnprice Jan 29, 2021

chrisbobbe Jan 29, 2021 •

edited

Loading

chrisbobbe Jan 29, 2021 •

edited

Loading

gnprice Jan 29, 2021

gnprice Jan 29, 2021

gnprice Jan 29, 2021

gnprice Jan 29, 2021

chrisbobbe Jan 29, 2021 •

edited

Loading

chrisbobbe Jan 29, 2021 •

edited

Loading

chrisbobbe commented Jan 29, 2021 •

edited

Loading

gnprice left a comment

gnprice Jan 29, 2021

gnprice Jan 29, 2021

gnprice Jan 29, 2021

chrisbobbe Jan 29, 2021 •

edited

Loading

chrisbobbe Jan 29, 2021

gnprice Jan 29, 2021 •

edited

Loading

gnprice Jan 29, 2021

chrisbobbe commented Jan 30, 2021

gnprice commented Jan 30, 2021

gnprice commented Jul 13, 2021

		showsHorizontalScrollIndicator={false}
		showsVerticalScrollIndicator={false}

Put lightbox header/footer text and buttons in the safe area. #4442

Put lightbox header/footer text and buttons in the safe area. #4442

Conversation

chrisbobbe commented Jan 28, 2021 • edited Loading

gnprice 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

chrisbobbe Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Jan 29, 2021 • edited Loading

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 Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented Jan 29, 2021 • edited Loading

gnprice 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

chrisbobbe Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jan 30, 2021

gnprice commented Jan 30, 2021

gnprice commented Jul 13, 2021

chrisbobbe commented Jan 28, 2021 •

edited

Loading

chrisbobbe Jan 29, 2021 •

edited

Loading

chrisbobbe Jan 29, 2021 •

edited

Loading

chrisbobbe Jan 29, 2021 •

edited

Loading

chrisbobbe Jan 29, 2021 •

edited

Loading

chrisbobbe Jan 29, 2021 •

edited

Loading

chrisbobbe commented Jan 29, 2021 •

edited

Loading

chrisbobbe Jan 29, 2021 •

edited

Loading

gnprice Jan 29, 2021 •

edited

Loading