Fix some bugs Tim reported in share-to-Android experience. #5098

chrisbobbe · 2021-11-03T00:46:19Z

This doesn't include a fix for one of them; I've filed #5097 for that.

Tim said:

For sending that message, I used the Android sharing feature for the first time. I like the UI it took me to but ran into a few input issues:

The default keyboard setting for the topic only was sentence case (which is usually not how we spell topics)

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.

The typeahead for the stream seemed to only do prefix match, so that typing just "sf" didn't find any matches.

gnprice

Thanks! Comment below.

share to android: Enforce mandatory-topics on submit, not during compose

Let's leave android out of the prefix here; this code is currently only reachable on Android, but it's code that will be in common with sharing to Zulip on iOS, once we do the iOS-specific work to make it reachable there too.

gnprice · 2021-11-03T23:09:22Z

src/sharing/ShareToStream.js

-    if (sharedData.type !== 'text') {
-      return stream !== '' && (topic !== '' || !mandatoryTopics);
-    }
-
-    return stream !== '' && (topic !== '' || !mandatoryTopics) && message !== '';
+    return sharedData.type !== 'text' || message !== '';


Hmm, this seems like a regression -- it means that when you actually have an empty topic, we'll show the send button as enabled even when that means you can't send, right?

And similarly for streams.

when you actually have an empty topic, we'll show the send button as enabled even when that means you can't send, right?

That's right. But I think this may actually be an improvement? It's good to tell the user specifically why they can't send a message, right, rather than having them guess (and possibly think it's a bug)?

I notice that the web app doesn't disable the send button, and waits for you to click it before giving you an alert. This is what I see just after clicking "Send":

One thing that I prefer in the web app's UX, for accessibility, is that the alert persists, right next to the relevant input, even as you go and change the value of the input. (Until you dismiss the alert or you fix the issue and click "Send".) When it's in a modal, like I have here in this revision, you have to keep the validation requirement in your working memory while you try to go fix the error.

But I guess I still think it's an improvement over the status quo, where there isn't a validation message at all.

Hmm interesting. It's true, it's good to be explicit about what's missing.

Still it seems like not good UX for the "send" button to be right there brightly beckoning to you when it's not actually possible to send. I don't think that's a good UX choice in the web app either.

For a quick comparison, I just opened up Gmail on Android and went to compose a new email. It actually does both:

When I haven't entered anything (even in the To: field), the "send" icon is grayed out.

But if I tap it anyway, I get a little modal error dialog: "Add at least one recipient.", with an "OK" button.

Then once I enter a valid email address, the icon gets its normal blue color.

Greg reports the following behavior in Gmail on Android [1]: > * When I haven't entered anything (even in the To: field), the > "send" icon is grayed out. > * But if I tap it anyway, I get a little modal error dialog: "Add > at least one recipient.", with an "OK" button. > * Then once I enter a valid email address, the icon gets its > normal blue color. So, do like that here: tell the ZulipButton that taps are handled when it's disabled, and handle them by giving an explicit validation-error message. [1] zulip#5098 (comment)

chrisbobbe · 2021-11-16T07:35:02Z

Thanks for the review! Revision pushed.

Greg reports the following behavior in Gmail on Android [1]: > * When I haven't entered anything (even in the To: field), the > "send" icon is grayed out. > * But if I tap it anyway, I get a little modal error dialog: "Add > at least one recipient.", with an "OK" button. > * Then once I enter a valid email address, the icon gets its > normal blue color. So, do like that here: tell the ZulipButton that taps are handled when it's disabled, and handle them by giving an explicit validation-error message. [1] zulip#5098 (comment)

But with prefix matches shown at the top.

In order to identify a topic as empty, we first have to take the raw input and .trim() it, and then compare the result of that to the empty string. That way, topics like " " and " " will correctly be treated as empty. For the trimming part, we've just been calling .trim() every time the input value changes and immediately setting the input value to the output of that .trim(). This had the annoying side effect that it was really frustrating to set a topic with multiple words in it, like "test topic". When you'd typed as far as "test ", it would trim out the trailing space. So, without a workaround, you'd end up with "testtopic". Then, if you wanted, you could move the cursor back in between the words and insert a space, to make "test topic". (That space wouldn't be trimmed out because it's not at the beginning or end of the string.) So, get rid of the side effect of changing the input value.

With the same reasoning as we used for doing this to the topic input, in a recent commit.

Greg reports the following behavior in Gmail on Android [1]: > * When I haven't entered anything (even in the To: field), the > "send" icon is grayed out. > * But if I tap it anyway, I get a little modal error dialog: "Add > at least one recipient.", with an "OK" button. > * Then once I enter a valid email address, the icon gets its > normal blue color. So, do like that here: tell the ZulipButton that taps are handled when it's disabled, and handle them by giving an explicit validation-error message. [1] zulip#5098 (comment)

gnprice · 2021-11-16T22:32:16Z

Thanks! Looks good -- merging.

chrisbobbe · 2021-11-16T22:36:59Z

Thanks for the review!

Greg reports the following behavior in Gmail on Android [1]: > * When I haven't entered anything (even in the To: field), the > "send" icon is grayed out. > * But if I tap it anyway, I get a little modal error dialog: "Add > at least one recipient.", with an "OK" button. > * Then once I enter a valid email address, the icon gets its > normal blue color. So, do like that here: tell the ZulipButton that taps are handled when it's disabled, and handle them by giving an explicit validation-error message. [1] zulip#5098 (comment)

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.

chrisbobbe added a-Android a-share-to Sharing photos/etc. into Zulip labels Nov 3, 2021

chrisbobbe requested a review from gnprice November 3, 2021 00:46

chrisbobbe force-pushed the pr-share-android-fixes branch from dd717d0 to 68c37bc Compare November 3, 2021 01:04

gnprice reviewed Nov 3, 2021

View reviewed changes

chrisbobbe force-pushed the pr-share-android-fixes branch from 68c37bc to 88dafcc Compare November 4, 2021 21:27

chrisbobbe force-pushed the pr-share-android-fixes branch from 88dafcc to 69b49b4 Compare November 16, 2021 07:33

chrisbobbe force-pushed the pr-share-android-fixes branch from 69b49b4 to e6501ea Compare November 16, 2021 08:00

chrisbobbe added 7 commits November 16, 2021 14:31

StreamAutocomplete: Include non-prefix matches

85f9330

But with prefix matches shown at the top.

share-to: Stop mutating input value for stream-not-empty enforcement

7889a50

With the same reasoning as we used for doing this to the topic input, in a recent commit.

share-to: Replace isSendButtonEnabled with getValidationErrors

562cf74

ZulipButton: Take isPressHandledWhenDisabled prop

bb90a7e

compose: Stop suggesting sentence case for topic input

9125fd2

gnprice force-pushed the pr-share-android-fixes branch from e6501ea to 9125fd2 Compare November 16, 2021 22:32

gnprice merged commit 9125fd2 into zulip:main Nov 16, 2021

chrisbobbe deleted the pr-share-android-fixes branch November 17, 2021 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some bugs Tim reported in share-to-Android experience. #5098

Fix some bugs Tim reported in share-to-Android experience. #5098

chrisbobbe commented Nov 3, 2021 •

edited

Loading

gnprice left a comment

gnprice Nov 3, 2021

chrisbobbe Nov 4, 2021 •

edited

Loading

gnprice Nov 5, 2021

chrisbobbe commented Nov 16, 2021

gnprice commented Nov 16, 2021

chrisbobbe commented Nov 16, 2021

Fix some bugs Tim reported in share-to-Android experience. #5098

Fix some bugs Tim reported in share-to-Android experience. #5098

Conversation

chrisbobbe commented Nov 3, 2021 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

gnprice Nov 3, 2021

Choose a reason for hiding this comment

chrisbobbe Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

gnprice Nov 5, 2021

Choose a reason for hiding this comment

chrisbobbe commented Nov 16, 2021

gnprice commented Nov 16, 2021

chrisbobbe commented Nov 16, 2021

chrisbobbe commented Nov 3, 2021 •

edited

Loading

chrisbobbe Nov 4, 2021 •

edited

Loading