Change streams tabs to a stack, initially show subscribed only, issue #5424 by nol13 · Pull Request #5443 · zulip/zulip-mobile

nol13 · 2022-07-17T21:59:50Z

Resolves #5424

The heading is still the basic header with blue text, wasn't sure if it needed any styling yet, or just to wait for the new header that's shown in the mock.

Same with the separator above the button, which has a gray spacer in the mock. I started to update the separator, and update the separators between the sections to show the "Pinned" and "Active" text above each section, but since it still only has "Pinned" and "Unpinned" sections I left it as is for now.

For the "All Streams" button I was going to use the NestedNavRow component but there were a few styling differences. Possibly NestedNavRow could be updated to work here too, or if this button will be reused, break it out into a component.

gnprice

Thanks @nol13! This looks good -- glad to have this change.

In our Git commit style, each commit should be a single coherent change with a clear commit message. (That helps make it easier to review, and also helps a lot with understanding what happened when reading the Git history.) In this case, I think that means squashing the changes together into a single commit. So please do that, and then give it a commit message in our format.

Please also post screenshots on this PR thread of the main parts of the UI that were changed. That's also helpful for reviewing, particularly for folks who don't have the mobile dev environment open all day to give review on the UX and design.

Other than that, just a few comments below, most of them quite small.

gnprice · 2022-07-18T22:58:14Z

+  rightItem: {
+    marginLeft: 'auto',
+  },


nit: It looks like this bit doesn't get used.

gnprice · 2022-07-18T23:02:46Z

+  },
+  streamsText: {
+    textTransform: 'uppercase',
+    color: '#666699',


Interesting, was this color specified in the design prototype? I don't think I'd seen specific colors mentioned, except in a couple of places related to stream colors.

When I switch to a dark theme, it looks like this color doesn't get good contrast. Presumably as the new design gets to a later stage, Vlad will work out a pair of colors to use here for the light and dark themes.

For now, let's do the same thing here as NestedNavRow does and use the theme's color property. (In NestedNavRow, this appears directly for the icons, and is handled for the text down in ZulipText.) That's a color chosen to have good contrast against the respective theme's background color.

Ah, ya I probably should have asked about that. I used an eyedropper tool to pull the color from the prototype. Sounds good.

gnprice · 2022-07-18T23:04:42Z

+  return (
+    <Touchable onPress={handlePressAllScreens}>
+      <View style={[appStyles.listItem, styles.allStreamsButton]}>
+        <ZulipTextIntl style={styles.streamsText} text="All Streams" />


nit: Let's make the text "All streams". That's our usual style for UI text; here it doesn't affect what's displayed, because of the all-caps transform, but it does make for a small bit less work for our translation contributors because we already have the string "All streams".

gnprice · 2022-07-18T23:27:07Z

On your questions above:

For the "All Streams" button I was going to use the NestedNavRow component but there were a few styling differences. Possibly NestedNavRow could be updated to work here too, or if this button will be reused, break it out into a component.

Cool -- thanks for explaining your reasoning, and the approach in this PR looks good to me.

I expect that in a future where we've completed the UI redesign that that prototype is part of, we'll have a lot more places in the UI that are similar to this and we'll want a reusable component. But for now, I suspect using NestedNavRow verbatim wouldn't look right in this context -- it's designed for a somewhat different context, like a settings UI. So having a local ad-hoc component sounds good.

The heading is still the basic header with blue text, wasn't sure if it needed any styling yet, or just to wait for the new header that's shown in the mock.

Yeah, I think this version works OK though it doesn't look pretty. Once you post screenshots, it'll be easier to see if others have suggestions for tweaks to its styling. Definitely we'll stick to something simple there for now.

Same with the separator above the button, which has a gray spacer in the mock. I started to update the separator, and update the separators between the sections to show the "Pinned" and "Active" text above each section, but since it still only has "Pinned" and "Unpinned" sections I left it as is for now.

Yeah. Changing the set of sections is an issue of its own: #5425. I think leaving the separators as-is is a good solution for now.

Thanks again!

gnprice · 2022-07-18T23:27:27Z

BTW for everyone's ease of reference, the mock/prototype we're discussing here is the one posted in this message:
https://chat.zulip.org/#narrow/stream/48-mobile/topic/UI.20redesign/near/1393657

The relevant screen from it is here:

nol13 · 2022-07-19T03:12:03Z

This removes StreamTabsScreen and sets the streams tab in MainTabsScreen to a stack navigator, with SubscriptionsScreen on top, and a button at the bottom of SubscriptionsScreen to navigate to StreamListScreen. I have tested this in an Android emulator but I have not yet run it on iOS. Fixes: zulip#5424

gnprice · 2022-07-19T22:07:03Z

Thanks @nol13 for the revision! This looks good -- merging.

chrisbobbe · 2022-07-21T23:03:19Z

Thanks, both! I noticed the changes when developing on main and had a few thoughts. 🙂

[NestedNavRow is] designed for a somewhat different context, like a settings UI

I think I wouldn't say that it's only for use in settings UIs. I guess I agree, in theory, that it could use some narrowing of its interface, and we might even make another reusable component one day. Here's the jsdoc:

/**
 * A button that navigates to a "nested" screen.
 *
 * Shows a right-facing arrow to indicate its purpose. If you need a
 * selectable option row instead, use `SelectableOptionRow`.
 */

Still, NestedNavRow is the primary place we define a UI metaphor—a right-facing arrow to navigate to a nested screen—and this new code depends on and reinforces the same metaphor. For that reason, would it make sense to have bidirectional code comments between here and NestedNavRow? I think that could be helpful, if we're not planning to just reuse NestedNavRow.

NestedNavRow got its current name and jsdoc thanks to a cleanup of mixed UI metaphors. The bug fixed in f3a9a0a / cfc9308 (consecutive commits) was that we were misusing this component in a list of selectable options; screenshots at #4683 (comment).

For the "All Streams" button I was going to use the NestedNavRow component but there were a few styling differences. Possibly NestedNavRow could be updated to work here too, or if this button will be reused, break it out into a component.

Looking closer, I think the differences in style are mostly due to warts in NestedNavRow that don't even serve its current callers well. The important exception is that we've got bold and all-caps here, and NestedNavRow's callers don't. But I wouldn't mind having bold and all-caps across the board, I think. Or letting callers configure that, with an interface that helps callers decide which way they should configure it.

I'll send a PR that cleans up the styling warts, and I'll include a commit that switches this site to use NestedNavRow. I think it could be really nice to not have to duplicate this code ad-hoc like this. EDIT: #5446

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-07-21T23:56:45Z

I think I wouldn't say that it's only for use in settings UIs.

I definitely agree it's not just for settings UIs. What I was thinking was something like: in this screen, we have the app-provided UI text "All streams" appearing in the same scrollable list with user-provided text, the names of streams. If the text were styled the same way, then it'd be likely to blend in almost as if it were another stream named "All streams". So having a more emphasized style of text here is helpful for contrasting it with the streams.

Whereas in other UI contexts, no such contrast is needed because the other text that it's parallel to really is similar in its character. For example, in a settings UI, where both this row and the other rows are app-provided UI text.

I'll send a PR that cleans up the styling warts, and I'll include a commit that switches this site to use NestedNavRow. I think it could be really nice to not have to duplicate this code ad-hoc like this. EDIT: #5446

Will take a look!

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)

nol13 changed the title ~~Nbk/m5424~~ Change streams tabs to a stack, initially show subscribed only, issue #5424 Jul 18, 2022

gnprice reviewed Jul 18, 2022

View reviewed changes

nol13 force-pushed the nbk/M5424 branch from 672cc73 to 9d200b8 Compare July 19, 2022 03:07

gnprice force-pushed the nbk/M5424 branch from 9d200b8 to 9d11e78 Compare July 19, 2022 22:06

gnprice merged commit 9d11e78 into zulip:main Jul 19, 2022

chrisbobbe mentioned this pull request Jul 21, 2022

layout: Fix warts in NestedNavRow and SwitchRow #5446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

Change streams tabs to a stack, initially show subscribed only, issue #5424#5443
gnprice merged 1 commit intozulip:mainfrom
nol13:nbk/M5424

nol13 commented Jul 17, 2022

Uh oh!

gnprice left a comment

Uh oh!

gnprice Jul 18, 2022

Uh oh!

gnprice Jul 18, 2022

Uh oh!

nol13 Jul 19, 2022

Uh oh!

gnprice Jul 18, 2022

Uh oh!

gnprice commented Jul 18, 2022

Uh oh!

gnprice commented Jul 18, 2022

Uh oh!

nol13 commented Jul 19, 2022

Uh oh!

gnprice commented Jul 19, 2022

Uh oh!

chrisbobbe commented Jul 21, 2022 •

edited

Loading

Uh oh!

gnprice commented Jul 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

nol13 commented Jul 17, 2022

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

nol13 Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice commented Jul 18, 2022

Uh oh!

gnprice commented Jul 18, 2022

Uh oh!

nol13 commented Jul 19, 2022

Uh oh!

gnprice commented Jul 19, 2022

Uh oh!

chrisbobbe commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Jul 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrisbobbe commented Jul 21, 2022 •

edited

Loading