layout: Fix warts in NestedNavRow and SwitchRow #5446

chrisbobbe · 2022-07-21T23:05:36Z

And (experimental) use NestedNavRow in place of new duplicated code at the bottom of SubscriptionsScreen, from 9d11e78.

See #5443 (comment) for rationale.

chrisbobbe · 2022-07-21T23:21:36Z

Before / after:

chrisbobbe · 2022-07-21T23:24:06Z

src/common/SwitchRow.js


 const componentStyles = createStyleSheet({
  container: {
-    height: 56,
+    // Minimum touch target height (and width):
+    //   https://material.io/design/usability/accessibility.html#layout-and-typography
+    minHeight: 48,
  },
  icon: {
-    marginVertical: 8,
    textAlign: 'center',
    marginLeft: 8,
    marginRight: 16,


SwitchRow: Use container minHeight for height We care that the touch target is at least 48px in height, and we care that the row can expand to accommodate a larger system font size than the standard. This seems like a transparent way to accomplish both.

Hmm, I think I too lazily copy-pasted this reasoning from

NestedNavRow: Use container minHeight for height

. NestedNavRow is a touch target across its whole surface, so it needs the minHeight: 48. This component, SwitchRow, isn't, so it doesn't.

Yeah, I guess the touch target here is the Switch.

That does extend to the full height of the row after this change, so the effect is the same.

Perhaps set the height of the Switch to 48? And then leave the container without a height-related style prop?

That does extend to the full height of the row after this change, so the effect is the same.

The styles.listItem is applying top and bottom padding of 8px, so the switch isn't extending to the full height of the row. There's still row for 8px above the top of the switch, and for 8px below the bottom.

The inspector shows that on iOS the switch has height 31 and width 51. So in the vertical direction, my device is actually giving the row a computed height of 31 + 8 + 8 = 47.

To make this row be the same height as the others (48) with common device settings, I suppose we could bump that 47 up to 48 (and handle any discrepancy on Android) by allotting more space for the switch, yeah. The space would have min-height of 32, I guess, rather than 48 in your proposal.

Or I could apply minHeight: 48 to the row, and replace the current reasoning about touch targets with something like

To make this row be the same height as the others (48) with common device settings

The styles.listItem is applying top and bottom padding of 8px, so the switch isn't extending to the full height of the row. There's still row for 8px above the top of the switch, and for 8px below the bottom.

Hmm, I see.

Does that padding end up as part of the touch target? I think we want it to be. In that case, setting the minHeight of the row to 48px makes sense.

If the padding isn't part of the touch target, then since 32px isn't tall enough for the touch target we'll want to make the touch target taller.

alya · 2022-07-26T18:12:13Z

@chrisbobbe could you explain a bit what I should focus on for reviewing this PR? Thanks!

chrisbobbe · 2022-08-18T21:31:58Z

(Alya, Greg, and I discussed this in the office, and IIRC we liked these changes. The next step is a code review from Greg.)

gnprice · 2022-08-30T00:35:44Z

Thanks @chrisbobbe for these cleanups!

All looks good, except the small point you mentioned (and I just replied to) above.

chrisbobbe · 2022-08-30T23:38:39Z

Thanks for the review! Revision pushed, addressing your comment with the approach I mentioned at #5446 (comment).

chrisbobbe · 2022-08-31T01:26:20Z

From discussion in the office today, unfortunately we don't have a nice, non-janky way to make the touch target extend up and down, such that if you tapped in the area between two switches, it would be taken as an interaction with one of them. From experimentation, the latter behavior is what you get on Android and iOS in their system settings UIs.

Ah, well; this should be ready for another code review.

No callers currently use this. Might as well remove it, because it could invite excessive micromanaging of individual style attributes by callers. If SwitchRow wants to offer more customization in the future, it can do so with purpose-designed props that don't have that problem. (For an example of this approach, see the leftMargin and rightMargin props of ZulipTextButton.)

This `margin` attribute wasn't controlling the left or right margin; those get set by the more specific marginLeft and marginRight. (Verified experimentally.) So, use marginVertical, which is clearer about the style we end up with.

This continues our project of removing things from miscStyles. Go ahead and give these more appropriate names for the places they're used, while we're at it. We'll change the actual style attributes to better align with each consumer, soon.

We care that the touch target is at least 48px in height, and we care that the row can expand to accommodate a larger system font size than the standard. This seems like a transparent way to accomplish both.

The actual touch target on SwitchRow is the switch. That's a native platform UI component that we can't resize, and we leave it to the platform to render as it wants. The inspector showed 31px height for the switch on iOS in my testing. Might as well still give the container a minHeight of 48 for the reason given in the comment.

(To see this change, go to a caller that actually passes an icon: the "Pinned", "Muted", and "Notifications" switches in the stream-settings screen.) The icon is already given whitespace on its left, by the container's padding. The switch, on the right, has that same amount of whitespace, for the same reason. So the left-margin 8 on the icon, removed here, was giving 8px more whitespace on the left of this row than we have on the right. Fix it so we have the same amount on each side. 16px seems too much to separate the icon from the text. That's the same as the 16px left padding we have on the container. Bring it down to 8px, which is what we put between the stream icon and the stream name just above these SwitchRow callers, in StreamCard.

There should be the same amount of horizontal whitespace between - the left edge of the screen and the icon on the left, and - the right edge of the screen and the icon on the right. The container's 16px padding is already providing that. No need to add an extra 8 on the left and 16 on the right. There should be space between the icons and the text; 8px on each side is fine for that.

We might not keep this change, but I do like how it trims out duplicated code and guards against misuse of the UI metaphor that we primarily describe in NestedNavRow (i.e., a right-facing arrow to navigate to a nested screen). I've said more about this here: zulip#5443 (comment)

gnprice · 2022-08-31T18:55:57Z

Thanks! Ah well, indeed.

Looks good given that limitation of RN -- merging.

chrisbobbe requested a review from gnprice July 21, 2022 23:05

chrisbobbe commented Jul 21, 2022

View reviewed changes

chrisbobbe marked this pull request as ready for review July 21, 2022 23:27

chrisbobbe mentioned this pull request Jul 21, 2022

Change streams tabs to a stack, initially show subscribed only, issue #5424 #5443

Merged

chrisbobbe changed the title ~~ui: Fix warts in NestedNavRow and SwitchRow~~ layout: Fix warts in NestedNavRow and SwitchRow Jul 21, 2022

chrisbobbe requested a review from alya July 21, 2022 23:36

chrisbobbe mentioned this pull request Aug 18, 2022

settings: Add option to mark read on scroll only in conversation views #5469

Merged

chrisbobbe force-pushed the pr-switch-nested-nav-row-fixes branch from 5d620d1 to e14dac1 Compare August 30, 2022 23:37

chrisbobbe added 10 commits August 31, 2022 11:54

NestedNavRow: Use container minHeight for height

ce7855b

We care that the touch target is at least 48px in height, and we care that the row can expand to accommodate a larger system font size than the standard. This seems like a transparent way to accomplish both.

NestedNavRow: Memoize Icon/IconRight's theme-dependent style prop

84996fa

NestedNavRow [nfc]: Use jsdoc style for an interface comment

3de8b7a

gnprice force-pushed the pr-switch-nested-nav-row-fixes branch from e14dac1 to c052965 Compare August 31, 2022 18:55

gnprice merged commit c052965 into zulip:main Aug 31, 2022

chrisbobbe deleted the pr-switch-nested-nav-row-fixes branch August 31, 2022 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

layout: Fix warts in NestedNavRow and SwitchRow #5446

layout: Fix warts in NestedNavRow and SwitchRow #5446

chrisbobbe commented Jul 21, 2022 •

edited

Loading

chrisbobbe commented Jul 21, 2022

chrisbobbe Jul 21, 2022 •

edited

Loading

gnprice Aug 30, 2022 •

edited

Loading

chrisbobbe Aug 30, 2022 •

edited

Loading

chrisbobbe Aug 30, 2022 •

edited

Loading

gnprice Aug 30, 2022

alya commented Jul 26, 2022

chrisbobbe commented Aug 18, 2022

gnprice commented Aug 30, 2022

chrisbobbe commented Aug 30, 2022

chrisbobbe commented Aug 31, 2022

gnprice commented Aug 31, 2022

layout: Fix warts in NestedNavRow and SwitchRow #5446

layout: Fix warts in NestedNavRow and SwitchRow #5446

Conversation

chrisbobbe commented Jul 21, 2022 • edited Loading

chrisbobbe commented Jul 21, 2022

chrisbobbe Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

gnprice Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

gnprice Aug 30, 2022

Choose a reason for hiding this comment

alya commented Jul 26, 2022

chrisbobbe commented Aug 18, 2022

gnprice commented Aug 30, 2022

chrisbobbe commented Aug 30, 2022

chrisbobbe commented Aug 31, 2022

gnprice commented Aug 31, 2022

chrisbobbe commented Jul 21, 2022 •

edited

Loading

chrisbobbe Jul 21, 2022 •

edited

Loading

gnprice Aug 30, 2022 •

edited

Loading

chrisbobbe Aug 30, 2022 •

edited

Loading

chrisbobbe Aug 30, 2022 •

edited

Loading