user-status: Add UI for setting emoji status #5277

chrisbobbe · 2022-03-05T02:18:16Z

And a few other UI improvements there, and a new "Busy" option for #5255.

Still TODO: making the status emoji actually show up where we want it to :)

gnprice

Thanks @chrisbobbe! Generally this looks good. Comments below.

As I mentioned in the office on Friday, before merging I'd like to see the emoji status appear in at least one place in the UI: the profile screen that you get to this screen from, because otherwise when you complete this interaction it looks like setting the emoji didn't work. But I'd be happy to leave the rest of the UI for a separate PR.

gnprice · 2022-03-07T18:14:37Z

src/user-statuses/EmojiInput.js

+import type { EmojiType } from '../types';
+import { createStyleSheet, BORDER_COLOR } from '../styles';
+
+export type Value = null | {| +code: string, +name: string, +type: EmojiType |};


nit: type before code, because the code lives in a namespace determined by the type

(and then same thing where constructing these in UserStatusScreen.js)

gnprice · 2022-03-07T18:18:34Z

src/user-statuses/UserStatusScreen.js

  const { navigation } = props;

+  const zulipFeatureLevel = useSelector(getZulipFeatureLevel);
  const auth = useSelector(getAuth);
  const ownUserId = useSelector(getOwnUserId);
  const userStatusText = useSelector(state => getUserStatus(state, ownUserId).status_text);
+  const userStatusEmoji = useSelector(state => getUserStatus(state, ownUserId).status_emoji);

-  const [statusText, setStatusText] = useState<string | null>(userStatusText);
+  const [textInputValue, setTextInputValue] = useState<string>(
+    inputValueFromStatusText(userStatusText),
+  );
+  const [emojiInputValue, setEmojiInputValue] = useState<EmojiInputValue>(
+    inputValueFromStatusEmoji(userStatusEmoji),
+  );
  const _ = useContext(TranslationContext);


I like the organization of stanzas here: props, selectors, state, (then below this) callbacks.

A nit: the useContext fits best in the selectors stanza, rather than state. (Like the selectors, it's basically pulling some state from some ancestor component near the top of the React tree.)

gnprice · 2022-03-07T19:01:28Z

src/user-statuses/EmojiInput.js

+          ...(Platform.OS === 'ios'
+            ? {
+                borderWidth: 1,
+                borderColor: BORDER_COLOR,
+                borderRadius: 2,
+                padding: 8,
+              }
+            : Object.freeze({})),


Hmm interesting. Why are the border and padding only on iOS?

Ah right, this needs a comment (and should possibly change). It's meant to mimic Input, which has

const componentStyles = createStyleSheet({ input: { ...Platform.select({ ios: { borderWidth: 1, borderColor: BORDER_COLOR, borderRadius: 2, padding: 8, }, }), }, });

Ah, got it. IIRC that conditional is there in order to conform to platform conventions. This UI might call for something else, though.

gnprice · 2022-03-07T19:07:41Z

src/user-statuses/UserStatusScreen.js

  button: {
    flex: 1,


Is this flex: 1 still doing something? I'd guess that it should be deleted when the enclosing "button wrapper" View is deleted -- the one that had flexDirection: 'row' because there were two buttons sharing the horizontal space.

gnprice · 2022-03-07T19:09:52Z

static/translations/messages_en.json

@@ -241,7 +241,6 @@
  "Share failed": "Share failed",
  "Download complete": "Download complete",
  "Download file": "Download file",
-  "Update": "Update",


At a grep, it looks like we still have this string elsewhere:

$ rg -w Update src/ src/streams/EditStreamCard.js 91: text={isNewStream ? 'Create' : 'Update'}

gnprice · 2022-03-07T19:17:57Z

src/user-statuses/userStatusTextSuggestions.js

 // This is a separate file due to Prettier having issues with formatting unicode
 // Using `prettier-ignore` does not help.


Hmm, seems like this comment stops applying.

If so, we can also fold this file into UserStatusScreen.js.

gnprice · 2022-03-07T19:28:34Z

src/user-statuses/UserStatusScreen.js

+              title={
+                zulipFeatureLevel >= 86
+                  ? `${parseUnicodeEmojiCode(emoji.emoji_code)} ${translatedText}`
+                  : text


Hmm, doesn't seem right that this gets translated or not depending on the server feature level.

… Huh, were these actually getting translated before? … Looks like not, when I test it just now.

I think this should say translatedText in the "else" case. And perhaps a good prep commit would be to start translating, which I think is nicely orthogonal to using status emoji.

gnprice · 2022-03-07T19:32:21Z

src/user-statuses/UserStatusScreen.js

+            <SelectableOptionRow
+              itemKey={index}
+              title={
+                zulipFeatureLevel >= 86


Probably good to pull this comparison out as a local (of the render function, up by the selectors), like const serverSupportsEmojiStatus = …. Then that can get a TODO-server comment.

gnprice · 2022-03-07T19:33:40Z

src/user-statuses/UserStatusScreen.js

+                setTextInputValue(translatedText);
+                setEmojiInputValue(inputValueFromStatusEmoji(emoji));


If emoji statuses aren't supported…we do end up just cutting the emoji out of the status suggestion, which I guess is a small downside. For these older servers, we could have kept up what we do now: stick the status emoji at the start of the suggested text. But I think achieving that has a higher complexity cost than it's worth. I've never really used this UI, I'm not sure how common it is to do so, and people are still free to depart from the suggested text, including by adding emojis, by using the text input.

Yeah, agreed.

gnprice · 2022-03-07T19:38:25Z

src/user-statuses/userStatusTextSuggestions.js

-  '🏠 Working remotely',
-];
+const items: $ReadOnlyArray<Item> = [
+  [{ emoji_name: 'working_on_it', emoji_code: '1f6e0' }, 'Busy'],


Let's get these codes from unicodeCodeByName. That way there isn't a risk of the name and code disagreeing with each other, or getting out of sync if our names change.

As Greg suggests: zulip#5277 (comment)

chrisbobbe · 2022-03-08T18:34:27Z

Thanks for the review! Revision pushed.

As I mentioned in the office on Friday, before merging I'd like to see the emoji status appear in at least one place in the UI: the profile screen that you get to this screen from, because otherwise when you complete this interaction it looks like setting the emoji didn't work. But I'd be happy to leave the rest of the UI for a separate PR.

Makes sense. I put a commit to show the emoji status on that profile screen you're talking about (AccountDetails) before the commits that let the user change their emoji status, so we don't have any commits that seem broken in the way you describe.

Then, at the end of the branch, I've added commits for displaying emoji status in other places, so those commits can easily be split off into another PR.

gnprice

Thanks for the revision! Two small comments, and then I haven't yet read the five new commits at the end:

205fe61 UserItem: Show emoji status, if set
85e3a33 user-statuses model: Export getUserStatuses after all
e34896b msglist: Show sender's emoji status, if set
2379d97 msglist tests: Use more of our stock example data
5aa031f msglist tests: Add snapshots for sender emoji statuses

First I'll push a quick revision as mentioned below.

gnprice · 2022-03-08T20:41:55Z

src/user-statuses/UserStatusScreen.js

  const { navigation } = props;
+  const _ = useContext(TranslationContext);


UserStatusScreen [nfc]: Move `useContext` upward, with props As Greg suggests: https://github.com/zulip/zulip-mobile/pull/5277#discussion_r820978534

Just to be sure you saw: my suggestion was actually to put it with the selectors, not with the props. But here with the props is fine too.

Ah, indeed! I've gone and put it with the selectors for the next revision.

gnprice · 2022-03-08T20:56:43Z

src/user-statuses/UserStatusScreen.js

          <SelectableOptionRow
            key={item}
            itemKey={item}
-            title={item}
+            title={_(item)}
            selected={item === statusTextFromInputValue(textInputValue)}
            onRequestSelectionChange={itemKey => {
              setTextInputValue(_(itemKey));


UserStatusScreen: Translate status-text suggestions Huh, I guess we hadn't been calling _, even though the strings appear in messages_en.json.

Yeah. I think in fact it's buggier than that -- the setTextInputValue call does translate. So you have an option that says "📅 In a meeting", even though you've chosen a UI language of say German; but then you select it and the text input gets filled with "📅 Bei einem Treffen".

Then further: when you choose that, you don't get the "selected" checkmark on the option as intended, because the selected logic is comparing to the untranslated text.

Those bugs are all fixed in the subsequent commit. But it'd be good to fix them all (including the latter one) in this separate commit. That can also simplify the subsequent commit that's meant to be about status emoji.

I'll try pushing a quick revision that does that.

Ah, right! Thanks.

gnprice · 2022-03-08T21:11:49Z

OK, pushed that revision -- please take a look.

Meanwhile I'll go read those last five commits.

And do you want to mark the PR as non-draft? I believe it was draft only because of the emoji not being shown.

gnprice

And a few comments below on those new commits. Let's plan for those to go in a separate PR, as the first part is so close to merging.

gnprice · 2022-03-08T21:40:12Z

src/users/UserItem.js

+        <View style={componentStyles.textAndEmojiWrapper}>
+          <View>


Hmm, what's the story with this additional layer of View-wrapper?

Investigating, I think you can drop the textAndEmojiWrapper view. It's just setting things that are already present in the parent's style. What is needed, besides adding the emoji at the end, is to drop the textWrapper styling and make this name-plus-email View have no style -- the flex: 1 is being unhelpful.

Great! That's a nice simplification; thanks. (I tested it out on my iPhone and didn't see any problems.)

Ah: I must have forgotten to test 2ba88b6 (merged with this simplification) with an unread count.

Screenshot on my discarded revision (205fe61):

Screenshot on main:

FWIW here's what it looks like with the fix, if both lines of text are visible:

gnprice · 2022-03-08T21:48:35Z

src/webview/static/base.css

+.status-emoji {
+  height: 1.4rem;
+  margin-left: 2px;
+}


This spacing seems a bit tight:

What does web do?

(That's with a Unicode emoji. This might look different for image emoji.)

Web has a margin-left of 2px also:

Somehow 2px ends up looking less tight on web than on mobile; not sure why.

Perhaps web has a space between them in the HTML? I notice these templates (and the handy snapshots of what they produce) don't.

I think maybe web doesn't have a space between them in the HTML? If I'm reading it correctly:

https://github.com/zulip/zulip/blob/3264361f6305082ba780882194bcd5cca83351a9/static/templates/message_body.hbs#L7

<span class="sender_name auto-select" role="button" tabindex="0">{{msg/sender_full_name}}{{> status_emoji msg/status_emoji_info}}</span>

And a screenshot from Chrome devtools:

I think if there were a space, it would show up in the devtools like "Chris Bobbe ". I notice that those quote marks in the devtools don't render in the web app.

Hmm, I agree -- that appears to show there's no space in the HTML.

gnprice · 2022-03-08T22:01:39Z

src/user-statuses/userStatusesModel.js

+export const getUserStatusFromModelState = (state: UserStatusesState, userId: UserId): UserStatus =>
+  state.get(userId, kUserStatusZero);


Let's put this next to the definition of the state type itself, where that fact about kUserStatusZero lives. I.e. let's put it in userStatusesCore.js.

Huh interesting. In its new location there, maybe it doesn't have to have this super long name? Maybe it can just be getUserStatus. (It couldn't be that if it stayed in userStatusesModel.js because that name would collide with the existing selector there.)

That would work well under a convention where we used these names relative to the modules they're in -- like import * as userStatusesCore from …. But our usual convention is instead to import the individual names directly. Under that convention, it's pretty helpful that we keep the individual names unique -- when the names are ambiguous, it gets confusing.

We've currently got statusText: string | null so this new code gives the same results, but it's clearer.

…state We've *said* statusText is `string | null` (changed from `string | void` in our recent model rework, 2106381). I would like to make it `string`, to match the `value` prop of the `Input` component type. That seems like a responsible thing for UserStatusScreen to do, since it's using the Input as a controlled input: https://reactjs.org/docs/forms.html#controlled-components Before we do that, we should at least fix this inconsistency, where the value of the input corresponds to "unset" (empty string) but statusText doesn't represent "unset" the way it says it will (null). It hasn't been a live bug, and this is NFC, because it just so happens that api.updateUserStatus converts to the empty string anyway, to talk to the server.

UserStatusScreen has undertaken to use Input as a controlled input, where at all times it's telling the Input what `value` it should have. https://reactjs.org/docs/forms.html#controlled-components It seems like the responsible thing for UserStatusScreen to do, for clarity, is to at least store the value meant for `value` in a proper state variable. We'll solidify that role with a rename, coming next.

We can drop "status" because it's implied (this is the UserStatusScreen). And it's good to be clear that we're working with the current value of the input, which could be different from the status text that's actually in effect, that the server knows about.

These are nice and simple, and we could probably do fine without them. But it sets a good pattern for the emoji input, coming soon.

This seems helpful. It's easy to end up with leading or trailing spaces in a text input, and want them to be treated as though they weren't there. The server may even trim the text status too, I'm not sure. Note that we don't do this by fussing with the actual value of the text input. That'd be easy to do, since we're using Input as a controlled input -- but it would be a misuse of that control. In particular, we'd introduce a bug like the following, on a topic input, fixed in zulip#5098: > Some bug prevented adding a space at the end of what I was typing > specifically on the topic and maybe stream input (the space would > appear and then immediately disappear, I think) ... so I was only > able to create the topic "bog plants" by typing "bogplants" and > then moving my cursor.

We don't like using NavigationService (zulip#4417), so this is nice to be able to do. Not *quite* NFC: if we somehow manage to have two consecutive EmojiPickerScreens at the top of the stack, we'll now just pop one of them instead of both, because we lose `navigateBack`'s special logic with its `sameRoutesCount` value. But that logic is designed for `ChatScreen` -- we do expect to have multiple of `ChatScreen`s at the top of the stack sometimes. We don't expect that with `EmojiPickerScreen`s.

…nput For now, just leave the status-text input by itself, filling all of the new View. And move its margin to the new View; we'll want it to apply to the emoji input too, coming soon.

As Greg suggests: zulip#5277 (comment)

Taking care not to send the status-emoji params to the server if the server doesn't support status emoji.

This doesn't yet change the main behavior of the button when pressed, i.e., we still clear the inputs *and* tell the server that we want to unset the status. Soon, we'll have it match the web app by only clearing the inputs. Then the user can apply the change with the submit button, which we'll rename from "Update" to "Save".

…icon I think this is much more common wording. And the checkmark icon is potentially confusing because it looks just like the checkmark on one of the status suggestions (SelectableOptionRow), if you've selected one. And it just doesn't feel normal to have an icon at all for a simple "OK" button like this.

This is another place where it's nice to use our status <-> input-value conversion functions. The logic is not complicated -- "Has the emoji or text been touched?" -- so it's nice to see this code doesn't have ||, ??, and () ? () : () all over the place.

When available, that is. Support was added in FL 86. If emoji statuses are supported, we use the suggested emoji for the emoji status, and the suggested text for the text status. If emoji statuses aren't supported…we do end up just cutting the emoji out of the status suggestion, which I guess is a small downside. For these older servers, we could have kept up what we do now: stick the status emoji at the start of the suggested text. But I think achieving that has a higher complexity cost than it's worth. I've never really used this UI, I'm not sure how common it is to do so, and people are still free to depart from the suggested text, including by adding emojis, by using the text input.

Fixes: zulip#5255

chrisbobbe · 2022-03-09T02:32:46Z

Great, thanks for the review! Revision pushed, with those last 5 commits set aside for a separate PR.

gnprice · 2022-03-09T02:57:00Z

Looks good! Merging.

One UI point that would be good to fix in the next PR, but I think isn't a blocker to merge: the status emoji end up looking faded, in both the selection UI and the profile screen. Like this:

Note the bright blue where the emoji appears in the list of suggestions, and the translucent-looking faded blue where it appears above. And similarly on the profile screen:

chrisbobbe · 2022-03-09T04:09:16Z

Thanks for the reviews! Just sent #5283.

See the code comments, and see screenshots of the bug at zulip#5277 (comment)

See the code comments, and see screenshots of the bug at zulip#5277 (comment) And Greg's suggestion to use this spacer View, to help keep the code simpler: zulip#5297 (comment)

chrisbobbe requested a review from gnprice March 5, 2022 02:18

chrisbobbe marked this pull request as draft March 5, 2022 02:22

gnprice reviewed Mar 7, 2022

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Mar 8, 2022

UserStatusScreen [nfc]: Move useContext upward, with props

1c543ca

As Greg suggests: zulip#5277 (comment)

chrisbobbe force-pushed the pr-set-emoji-status-ui branch from 94ab441 to 5aa031f Compare March 8, 2022 18:31

gnprice reviewed Mar 8, 2022

View reviewed changes

gnprice force-pushed the pr-set-emoji-status-ui branch from 5aa031f to 42310c9 Compare March 8, 2022 21:11

gnprice reviewed Mar 8, 2022

View reviewed changes

chrisbobbe added 20 commits March 8, 2022 18:15

UserStatusScreen [nfc]: Make a conditional more transparent

e6e8f21

We've currently got statusText: string | null so this new code gives the same results, but it's clearer.

UserStatusScreen [nfc]: Pull out status <-> input-value converters

32a127d

These are nice and simple, and we could probably do fine without them. But it sets a good pattern for the emoji input, coming soon.

EmojiPickerScreen: Put callback in route params

7a8208b

Emoji [nfc]: Allow setting size

96901e3

UserStatusScreen [nfc]: Add "input row" View, to soon include emoji i…

bbba246

…nput For now, just leave the status-text input by itself, filling all of the new View. And move its margin to the new View; we'll want it to apply to the emoji input too, coming soon.

UserStatusScreen [nfc]: Move useContext upward, with selectors

cf1a32c

As Greg suggests: zulip#5277 (comment)

UserStatusScreen: Translate status-text suggestions

261341c

AccountDetails: Show status emoji, if set

c7013e3

UserStatusScreen: Add and use EmojiInput, for status emoji

254b00f

Taking care not to send the status-emoji params to the server if the server doesn't support status emoji.

UserStatusScreen: Have clear button only clear the inputs, not save too

5b0e2f8

UserStatusScreen: Add "Busy" option

6d62335

Fixes: zulip#5255

chrisbobbe force-pushed the pr-set-emoji-status-ui branch from 42310c9 to 6d62335 Compare March 9, 2022 02:31

chrisbobbe marked this pull request as ready for review March 9, 2022 02:33

gnprice merged commit 6d62335 into zulip:main Mar 9, 2022

chrisbobbe deleted the pr-set-emoji-status-ui branch March 9, 2022 03:11

chrisbobbe mentioned this pull request Mar 9, 2022

Display emoji statuses in more places! #5283

Merged

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Mar 15, 2022

UserItem: Fix recent regression where UnreadCount was misaligned

77da5b2

See the code comments, and see screenshots of the bug at zulip#5277 (comment)

chrisbobbe mentioned this pull request Mar 15, 2022

UserItem: Fix recent regression where UnreadCount was misaligned #5297

Merged

		// This is a separate file due to Prettier having issues with formatting unicode
		// Using `prettier-ignore` does not help.

		setTextInputValue(translatedText);
		setEmojiInputValue(inputValueFromStatusEmoji(emoji));

		const { navigation } = props;
		const _ = useContext(TranslationContext);

		export const getUserStatusFromModelState = (state: UserStatusesState, userId: UserId): UserStatus =>
		state.get(userId, kUserStatusZero);

user-status: Add UI for setting emoji status #5277

user-status: Add UI for setting emoji status #5277

Conversation

chrisbobbe commented Mar 5, 2022

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

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 commented Mar 8, 2022 • 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

gnprice commented Mar 8, 2022

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Mar 15, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Mar 9, 2022 • 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 commented Mar 9, 2022

gnprice commented Mar 9, 2022

chrisbobbe commented Mar 9, 2022 • edited Loading

chrisbobbe commented Mar 8, 2022 •

edited

Loading

chrisbobbe Mar 9, 2022 •

edited

Loading

chrisbobbe Mar 15, 2022 •

edited

Loading

chrisbobbe Mar 9, 2022 •

edited

Loading

chrisbobbe commented Mar 9, 2022 •

edited

Loading