More prep work for handling safe areas. #4683

chrisbobbe · 2021-04-20T20:30:08Z

I think a good chunk of the work toward making #3066 work without things looking too awkward will be to add appropriate left and right padding to various row elements that are meant to extend to the left and right edges of the screen.

For example, we don't want this (how it is now):

But we also don't want to just throw everything in a View with its own margin or padding—rows are cut off abruptly and don't extend to the edge; you can see this in the stream header, but it also affects the touch highlight (not shown):

Instead, we want this—rows extend to the edge, but content is padded so it doesn't get hidden:

(For the left inset there, it looks to me like we're wasting some space—but the padding I added is exactly the length of the inset announced by the device. While there's no notch there, it seems Apple has decided it needs a large inset because of how rounded the corners are, which I guess makes sense.)

This PR does

a bit of cleanup on some of our row components,
some other things toward Safe areas are not handled correctly on iPhone X #3066 (like making UserPickerCard a function component), and
some independent cleanups.

Then I've got a draft on top of this that adds padding for (I think) all the relevant "row" components.

I'll post screenshots of how the appearance changes in this PR (it's not a lot) soon.

chrisbobbe · 2021-04-20T20:37:26Z

The icon size changes from these two commits:

79d9b2e NavButton: Use standard icon size of 24, instead of using 26.
d4475f7 TitleStream: Use standard icon size of 24, instead of 20, for StreamIcon.

Before this PR (left); after this PR (right):

chrisbobbe · 2021-04-20T20:42:28Z

The change to the user-status-suggestion list from

361b0db UserStatusScreen [nfc]: Use new SelectableOptionRow.

Before this PR:

After this PR:

WesleyAC

Looks good overall, just a few comments!

For the commit messages in the SelectableOptionRow [nfc]: Widen interface to match name series, could you add more description to each commit? It'd be useful to have a summary of what each one does.

And for the /n commits, it'd be good to replace those with the final number, although I presume you're planning to do that already?

WesleyAC · 2021-05-07T18:20:17Z

src/common/SelectableOptionRow.js

@@ -0,0 +1,51 @@
+/* @flow strict-local */


It would be good to include in the commit message here what you intend to use this component for, it's not clear to me from the name.

WesleyAC · 2021-05-07T18:25:55Z

src/common/SelectableOptionRow.js

@@ -33,8 +33,8 @@ type Props<TItemKey: string | number> = $ReadOnly<{|
  // position that requires it).
  itemKey: TItemKey,

-  subtitle: string,


Could this commit message be SelectableOptionRow [nfc]: Reorder variables. instead of SelectableOptionRow [nfc]: Tiny refactor.? That makes it more clear what's going on.

WesleyAC · 2021-05-07T18:30:34Z

src/common/NavButtonRow.js

+ * Shows a right-facing arrow to indicate its purpose. If you need a
+ * selectable option row instead, use `SelectableOptionRow`.
+ */
+export default class NavButtonRow extends PureComponent<Props> {


IMO NavButtonRow is a confusing name for this, maybe NestedNavRow or something similar? NavButton makes me think of a button in the NavBar, which this isn't.

Good point; NestedNavRow sounds good.

WesleyAC · 2021-05-07T18:34:47Z

src/user-picker/UserPickerCard.js

-      );
+    if (this.state.selected.length > prevState.selected.length) {
+      setTimeout(() => {
+        if (this.listRef) {


Could you this.listRef?.scrollToEnd(); instead?

WesleyAC · 2021-05-07T18:36:53Z

src/react.js

@@ -0,0 +1,20 @@
+/* @flow strict-local */


Should this live in src/util/react.js instead? That seems like maybe a more apt name.

I'd been following the pattern of src/react-redux.js, src/react-native-safe-area-context.js, and src/react-navigation.js (possibly others?). These have so far been for "type wrappers"; things like

/** * Exactly like the `useSelector` in `react-redux` upstream, but more typed. * […]

But

Maybe that's not the best place for those?

This one isn't a type wrapper; it's not a wrapper around an existing bit of API.

So, putting it in utils sounds good.

I've generally avoided putting new things in src/utils/, because I tend to feel that a "utils" directory makes things harder to find more than it makes them easier. It makes the list of things in the parent directory shorter; but now when you want to browse for something, the first thing you have to guess is whether it got called a "utility". Better for the top-level organization to be by what general area or subsystem the code belongs to.

If you take a look in the server/webapp repo, you'll see there are no directories with "util" names there, either. But there is this pattern, which gives me an idea here:

$ git ls-files | grep util static/js/hash_util.js static/js/keydown_util.ts static/js/list_util.js static/js/message_util.js …

So for example there are a lot of files related to messages; and one of them, for some miscellaneous functions with minimal dependencies, is named message_util.js. That feels a lot better to me than a util/messages.js would, because you can still navigate it by starting with "the thing I'm looking for would be about messages".

Here, how about src/reactUtils.js?

chrisbobbe · 2021-05-10T16:06:29Z

Thanks for the review, @WesleyAC! Revision pushed.

And for the /n commits, it'd be good to replace those with the final number, although I presume you're planning to do that already?

Greg has often not gone back to do that; see for example the series ending in 7bf6112. I think that's been seen as OK because the really essential thing is to know each commit's position in the sequence, and it's maybe less important to know the total number. That, and it can be a small pain, while developing a PR, to go and edit each commit message if you decide the total number of commits should change.

But I'm OK to start doing that, probably after a round or two of review so the sequence is more settled than in a first draft; I've done so in this revision. 🙂

chrisbobbe · 2021-06-25T00:26:40Z

(Just rebased.)

gnprice

Thanks @chrisbobbe and @WesleyAC ! This looks good -- small comments below, and ~~I'll reply on a thread above~~ #4683 (comment) .

src/user-picker/UserPickerCard.js

gnprice · 2021-07-08T05:00:25Z

src/title/TitleStream.js

-            size={20}
+            size={24}


This actually ends up looking kind of oversized to me, in your handy screenshot at #4683 (comment) -- like it's out of proportion with the text that comes after it.

I think perhaps it makes sense here to think of this as not our usual use of an icon. The reasoning in previous adjustments like this (like the commit before this one) is about icon buttons -- but this icon isn't a button, it's not something you can activate. Instead it's sort of a sigil attached to the stream name, and it wants to be in a size that corresponds to that text.

It looks like the existing code ends up using the same size here as the font size on the title. Perhaps just replace the literal 20 here with an explicit reference to that?

Use the icon component's `size` prop instead of `fontSize`. Like we did in 9a502e8; see discussion there. It looks like the `size={18}` wasn't doing anything: RNVI's implementation is such that the effect of passing a `fontSize` style is that anything passed for `size` is ignored; see https://github.com/oblador/react-native-vector-icons/blob/v7.0.0/lib/create-icon-set.js#L78. Also, remove `width` and `height` from the styles. With these set to `auto` (their default), React Native will calculate an appropriate width and height based on the element's content; see https://reactnative.dev/docs/flexbox#width-and-height. Although we *do* want those to be 24, setting them explicitly like this is a bit belt-and-suspenders.

Use the icon component's `size` prop, instead. Like we did in 9a502e8; see discussion there. And like we did in a recent commit in this series.

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 """

Greg points out [1], > [Our standard size of 24 for icon buttons] actually ends up > looking kind of oversized to me, in your handy screenshot at > zulip#4683 (comment) > -- like it's out of proportion with the text that comes after it. > > I think perhaps it makes sense here to think of this as not our > usual use of an icon. [...] Instead it's sort of a sigil attached > to the stream name, and it wants to be in a size that corresponds > to that text. > > It looks like the existing code ends up using the same size here > as the font size on the title. Perhaps just replace the literal 20 > here with an explicit reference to that? [1] zulip#4683 (comment)

The new name is more specific about the component's job: to show a labeled switch. The old name might also have suggested that the component represents one item among a set of closely related items, which it doesn't, necessarily [1]. It would have done in cases where we'd put it to use for showing a radio button [2], a checkbox [3], or a menu item [4], but those aren't possible in its current implementation. And that's probably for the best; this rename narrows the component's job, instead of widening it, on purpose. Radio buttons, checkboxes, and menu items are the most likely-looking things we might expand this component's job to cover -- but an item of those types probably wants to assume it belongs to a set of closely related items of its type (a checkbox in a list of checkboxes, etc.) when it chooses its implementation. [1] https://material.io/components/switches: "Switches toggle the state of a single item on or off." [2] https://material.io/components/radio-buttons: "...one option from a set". [3] https://material.io/components/checkboxes: "...one or more items from a set". [4] https://material.io/components/menus#usage: "...from multiple options".

Created by copy-pasting `LanguagePickerItem`. In the next several commits, we'll make this into a more general-purpose component that may be useful to represent a checkbox or a radio button. Then we'll switch `LanguagePickerItem`'s caller to use this component instead, and remove `LanguagePickerItem`. We'll also use it for the items on `UserStatusScreen`, which now incorrectly show right-facing arrows, as though they'll navigate somewhere. Future uses of this component (e.g., we may want it for zulip#4009) can help refine its interface. For now, just let that process start -- and make it easier to provide a consistent look-and-feel -- by making a reusable component.

`locale`, a string, was used as a unique key for identifying the item represented by the row. Rename it to `itemKey`, and let it be a string or a number, in case callers want to pass a number.

`name` was used for the translated name of the language, and it appeared just under the native name. For example, if you're viewing the app in English, you'd see an item like -------- Français French -------- where the first line is `nativeName` and the second is `name`. An interface like this seems useful for callers, except not all callers will be dealing with locale names. So, take `name` and give it the more generic name `subtitle`; we'll change `nativeName` to `title` in the next commit.

Change `nativeName` to `title`. For the same reason we changed `name` to `subtitle` in the previous commit.

This component won't only be used for items in a list of locales, so give this bit of styles a more generic name.

Which doesn't have the right-facing arrows; those have been misleading here because they make the user think we'll expand something or navigate somewhere when they interact with the item. See discussion: https://chat.zulip.org/#narrow/stream/48-mobile/topic/User.20status.20items.20look.20odd/near/1166000.

And give it a jsdoc.

Inline `this.listRef` in `componentDidUpdate`. Flow rightly complains that `this.listRef` might have been cleared before we try to call `.scrollToEnd` on it, hence the $FlowFixMe; we'll fix that soon.

Use the modern React ref API instead of the older callback style [1]. [1] https://reactjs.org/docs/refs-and-the-dom.html#callback-refs

chrisbobbe · 2021-07-08T14:46:53Z

Thanks for the review! Conflicts resolved and revision pushed. 🙂

gnprice · 2021-07-08T23:43:18Z

Looks good, thanks! Merging.

chrisbobbe · 2021-07-09T00:31:35Z

Thanks for the review!

Each of these UI elements is a row that has meaningful content that we need to keep within the safe areas, but the rest of the row (its distinct background color, its touchable area, etc.) is meant to extend through the insets to the extreme left and right of the screen. See example screenshots at zulip#4683 (comment). So, make that happen. We do so by giving the rows left and right padding. This is easy and makes the row elements' styles pretty intuitive, but it does mean we'll have to take care, in the next few commits, not to add any padding to the elements that contain the rows -- we don't want to double up the padding by mistake. (This requirement ends up being kind of annoying on screens that have a mix of these kinds of rows and regular elements. Possibly there's a better design pattern we haven't yet considered.) Related: zulip#3066

Each of these UI elements is a row that has meaningful content that we need to keep within the safe areas, but the rest of the row (its distinct background color, its touchable area, etc.) is meant to extend through the insets to the extreme left and right of the screen. See example screenshots at zulip#4683 (comment). So, make that happen. We do so by giving the rows left and right padding. This is easy and makes the row elements' styles pretty intuitive, but it does mean we'll have to take care, in the next few commits, not to add any padding to the elements that contain the rows -- we don't want to double up the padding by mistake. (This requirement ends up being kind of annoying on screens that have a mix of these kinds of rows and regular elements. But this is unfortunately the workable design we've found; see our architecture doc on handling safe areas.) Related: zulip#3066

chrisbobbe requested review from gnprice and WesleyAC April 20, 2021 20:30

chrisbobbe force-pushed the pr-safe-areas-prep branch from b2b612e to 3fe76d9 Compare April 20, 2021 20:41

chrisbobbe mentioned this pull request May 4, 2021

Open links in external browser #4679

Merged

chrisbobbe added the P1 high-priority label May 4, 2021

WesleyAC suggested changes May 7, 2021

View reviewed changes

chrisbobbe force-pushed the pr-safe-areas-prep branch from 3fe76d9 to c5ebe55 Compare May 10, 2021 16:00

chrisbobbe mentioned this pull request May 14, 2021

ServerCompatNotice: Add, to nag users connected to servers <2.0.0. #4750

Merged

chrisbobbe mentioned this pull request May 22, 2021

Translate status text suggestions #3432

Closed

chrisbobbe force-pushed the pr-safe-areas-prep branch from c5ebe55 to 6679619 Compare June 25, 2021 00:26

chrisbobbe mentioned this pull request Jun 30, 2021

Use 'SafeAreaView' through the app #3067

Closed

gnprice reviewed Jul 8, 2021

View reviewed changes

chrisbobbe added 14 commits July 8, 2021 10:16

OptionButton [nfc]: Use styles.settingsIcon more directly.

060fb11

OptionRow [nfc]: Use styles.settingsIcon more directly.

3a6896c

NavButton: Don't use fontSize to control icon size.

e8571d2

Use the icon component's `size` prop, instead. Like we did in 9a502e8; see discussion there. And like we did in a recent commit in this series.

EditStreamCard [nfc]: Follow recent OptionRow -> SwitchRow rename.

0b9ff5c

LanguagePickerItem [nfc]: Convert to a function component.

9b316f3

SelectableOptionRow [nfc]: Widen interface to match name (1/5).

de6c41b

`locale`, a string, was used as a unique key for identifying the item represented by the row. Rename it to `itemKey`, and let it be a string or a number, in case callers want to pass a number.

SelectableOptionRow [nfc]: Widen interface to match name (3/5).

20cb6f8

Change `nativeName` to `title`. For the same reason we changed `name` to `subtitle` in the previous commit.

SelectableOptionRow [nfc]: Widen interface to match name (4/5).

77e2a02

This component won't only be used for items in a list of locales, so give this bit of styles a more generic name.

chrisbobbe added 16 commits July 8, 2021 10:44

SelectableOptionRow [nfc]: Tighten event handler interface.

aad2962

SelectableOptionRow [nfc]: Comment about itemKey.

8f14439

SelectableOptionRow [nfc]: Reorder variables.

040b1f3

SelectableOptionRow: Make subtitle optional.

55ccae4

NestedNavRow [nfc]: Rename, from OptionButton.

cfc9308

And give it a jsdoc.

UserPickerCard [nfc]: Convert to function component (1/7).

0e4f0bc

UserPickerCard [nfc]: Convert to function component (2/7).

a7397b8

UserPickerCard [nfc]: Convert to function component (3/7).

d53ded9

UserPickerCard [nfc]: Convert to function component (4/7).

34ff815

UserPickerCard [nfc]: Convert to function component (5/7).

391118e

Inline `this.listRef` in `componentDidUpdate`. Flow rightly complains that `this.listRef` might have been cleared before we try to call `.scrollToEnd` on it, hence the $FlowFixMe; we'll fix that soon.

UserPickerCard: Put this.listRef nullish check in the right place.

ffff16f

UserPickerCard [nfc]: Make this.listRef nullish check more compact.

2c9735f

UserPickerCard [nfc]: Convert to function component (6/7).

d69dc45

Use the modern React ref API instead of the older callback style [1]. [1] https://reactjs.org/docs/refs-and-the-dom.html#callback-refs

react: Add usePrevious.

cc03c4f

UserPickerCard [nfc]: Convert to function component (7/7).

d5fce7f

chrisbobbe force-pushed the pr-safe-areas-prep branch from 6679619 to d5fce7f Compare July 8, 2021 14:45

gnprice merged commit d5fce7f into zulip:master Jul 8, 2021

chrisbobbe mentioned this pull request Jul 12, 2021

Handle safe areas (mostly). #4893

Draft

chrisbobbe deleted the pr-safe-areas-prep branch November 4, 2021 21:54

chrisbobbe mentioned this pull request May 17, 2022

mute: Request and consume new user_topic format in API; adapt our model #5382

Merged

chrisbobbe mentioned this pull request Jul 21, 2022

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

Merged

gnprice mentioned this pull request Nov 18, 2022

ChatScreen: Fix blank strip between compose box and keyboard on iOS #5564

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More prep work for handling safe areas. #4683

More prep work for handling safe areas. #4683

chrisbobbe commented Apr 20, 2021 •

edited

Loading

chrisbobbe commented Apr 20, 2021

chrisbobbe commented Apr 20, 2021

WesleyAC left a comment

WesleyAC May 7, 2021

WesleyAC May 7, 2021

WesleyAC May 7, 2021

chrisbobbe May 10, 2021

WesleyAC May 7, 2021

WesleyAC May 7, 2021

chrisbobbe May 10, 2021 •

edited

Loading

gnprice Jul 8, 2021

chrisbobbe commented May 10, 2021 •

edited

Loading

chrisbobbe commented Jun 25, 2021

gnprice left a comment •

edited

Loading

gnprice Jul 8, 2021

chrisbobbe commented Jul 8, 2021

gnprice commented Jul 8, 2021

chrisbobbe commented Jul 9, 2021

More prep work for handling safe areas. #4683

More prep work for handling safe areas. #4683

Conversation

chrisbobbe commented Apr 20, 2021 • edited Loading

chrisbobbe commented Apr 20, 2021

chrisbobbe commented Apr 20, 2021

WesleyAC 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

chrisbobbe May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented May 10, 2021 • edited Loading

chrisbobbe commented Jun 25, 2021

gnprice left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jul 8, 2021

gnprice commented Jul 8, 2021

chrisbobbe commented Jul 9, 2021

chrisbobbe commented Apr 20, 2021 •

edited

Loading

chrisbobbe May 10, 2021 •

edited

Loading

chrisbobbe commented May 10, 2021 •

edited

Loading

gnprice left a comment •

edited

Loading