Warn on @-mentioning someone who won't see it because not subscribed #4101

agrawal-d · 2020-05-12T08:18:13Z

Fixes #3373.

Summary of Changes

We use the endpoint /users/{user_id}/subscriptions/{stream_id} to get the subscription status.
On non-PM screens, we show a warning when someone unsubscribed is mentioned.
This warning shows up with a nice animation.
Pressing the subscribe button subscribes the user and dismisses the warning.
Tapping on the warning dismisses it.
Multiple warnings can be shown at a time.

Web app equivalent, for comparison:

agrawal-d · 2020-05-12T08:19:16Z

@chrisbobbe, @gnprice, @ray-kraesig this PR is ready for a review.

agrawal-d · 2020-05-13T11:02:43Z

Blocked until zulip/zulip#14966 is resolved.

agrawal-d · 2020-06-11T07:16:27Z

zulip/zulip#14966 is now fixed, and I can continue work on this.

agrawal-d · 2020-06-12T14:16:04Z

WIP, as I have to make some improvements, but can still be reviewed.

gnprice

Thanks @agrawal-d ! Some comments below.

I haven't read all the code in detail, or tried running it -- I figure that'll make more sense once you consider it ready.

src/api/subscriptions/getSubscriptionToStream.js

src/styles/composeBoxStyles.js

src/message/MentionedUserNotSubscribed.js

gnprice · 2020-06-12T21:23:36Z

src/autocomplete/AutocompleteView.js

  onAutocomplete: (input: string) => void,
+  processAutoComplete: (completion: string, completionType: string) => void,


Having two different callbacks here for the same event doesn't feel like the right interface. Better to adjust the one callback's interface to do what we need it to.

OK, I've modified the current callback.

gnprice · 2020-06-12T21:44:23Z

src/compose/ComposeBox.js

+  handleMentionSubscribedCheck = async (message: string) => {
+    const { usersByEmail, narrow, auth, streamId } = this.props;
+
+    if (isPrivateNarrow(narrow)) {
+      return;
+    }
+    const unformattedMessage = message.split('**')[1];
+
+    // We skip user groups, for which autocompletes are of the form
+    // `*<user_group_name>*`, and therefore, message.split('**')[1]
+    // is undefined.
+    if (unformattedMessage === undefined) {
+      return;
+    }
+    const [userFullName, userId] = unformattedMessage.split('|');
+    const unsubscribedMentions = this.state.unsubscribedMentions.slice();
+    let mentionedUser: UserOrBot;
+
+    // eslint-disable-next-line no-unused-vars
+    for (const [email, user] of usersByEmail) {
+      if (userId !== undefined) {
+        if (user.user_id === userId) {
+          mentionedUser = user;
+          break;
+        }
+      } else if (user.full_name === userFullName) {
+        mentionedUser = user;
+        break;
+      }
+    }
+    if (!mentionedUser || unsubscribedMentions.includes(mentionedUser)) {
+      return;
+    }
+
+    if (!(await api.getSubscriptionToStream(auth, mentionedUser.user_id, streamId)).is_subscribed) {
+      unsubscribedMentions.push(mentionedUser.user_id);
+      this.setState({ unsubscribedMentions });
+    }
+  };
+
+  handleMentionWarningDismiss = (user: UserOrBot) => {
+    this.setState(prevState => ({
+      unsubscribedMentions: prevState.unsubscribedMentions.filter(
+        (x: number) => x !== user.user_id,
+      ),
+    }));
+  };
+
+  clearMentionWarnings = () => {
+    this.setState({ unsubscribedMentions: [] });
+  };


The ComposeBox component already handles a ton of different things, and that's already made it a lot more challenging to understand than it ought to be. So I'd like to avoid adding more, and instead put new functionality in appropriate new components and have ComposeBox just do the minimum to wire them in.

Here, I think a good way to do that would be to have a component that

has zero or more individual MentionedUserNotSubscribed components as children

has the unsubscribedMentions state

has the implementations of these three methods

Then from ComposeBox we just need to be able to invoke handleMentionSubscribedCheck and clearMentionWarnings. I think the React way to do this is to have ComposeBox hold a "ref":
https://reactjs.org/docs/refs-and-the-dom.html
using createRef, that points to that child component.

(Alternatively the state could live up at ComposeBox, and it could just pass the child some callbacks to get at it. But that would expose somewhat more of the details to ComposeBox, and because of the situation where ComposeBox has too many details of too many things already, I'd rather avoid that.)

Done. The new component is MentionWarnings.

agrawal-d · 2020-06-15T13:06:56Z

@gnprice thanks for the review. I've pushed some changes.

src/compose/MentionWarnings.js

chrisbobbe

Thanks, @agrawal-d! 🙂 I've made a few comments. I agree with @gnprice that the ComposeBox component is at risk of
becoming even harder to understand, and I think this solution (using refs) is a good way to mitigate that.

With that being said, though, let's get some more comments in there. In particular, to put a spotlight on the methods of MentionWarnings (handleMentionSubscribedCheck and clearMentionWarnings, I think) that are meant to be part of the component's interface, especially since using refs is...not quite an antipattern, but something we don't do very often. 🙂

I suppose another way of not adding to ComposeBox's confusion would be to have MentionWarnings be a child of PeopleAutocomplete. (IIUC, MentionWarnings is only useful when you're doing an autocomplete, specifically a people autocomplete.) One problem with that is that PeopleAutocomplete doesn't have any idea of what narrow you're in (and why should it, otherwise?), and MentionWarnings depends on knowing that. Another problem is that it may not make sense from a graphical layout perspective (I haven't looked too closely at this but it sounds like a potential red flag). So I think the current solution is fine, but I wanted to bring this up in case it hasn't been considered yet.

One other thing: Do you think a cancel button would fit in there somewhere, like the "x" button in the web app? It would just call clearMentionWarnings, I think. People might get frustrated that the warnings are hiding the message list from view, with no way to dismiss them except to send a message or subscribe the user. Even better would be to automatically dismiss a warning if the user removes the mention of that user from the input text, if that's easy.

chrisbobbe · 2020-07-02T20:50:12Z

src/compose/ComposeBox.js

@@ -106,6 +111,7 @@ class ComposeBox extends PureComponent<Props, State> {

  messageInput: ?TextInput = null;
  topicInput: ?TextInput = null;
+  mentionWarnings: ?MentionWarnings = null;


Nit: Would it be possible to use the new ref API, React.createRef()? Here's a doc; looks like it's been available since React 16.3.

Not a big deal if not, but it's nice to experiment with new APIs and update when possible, and we haven't done so with this API yet. 🙂 (edit: Mm, looks like maybe we have tried before, but quite a while ago; there was some discussion in 2018.)

It might be tricky to get it to work with Flow, but from some experimentation, I believe this follows the new API and gives no Flow errors:

diff --git src/compose/ComposeBox.js src/compose/ComposeBox.js index 33a7443fb..df0417f18 100644 --- src/compose/ComposeBox.js +++ src/compose/ComposeBox.js @@ -111,7 +111,7 @@ class ComposeBox extends PureComponent<Props, State> { messageInput: ?TextInput = null; topicInput: ?TextInput = null; - mentionWarnings: ?MentionWarnings = null; + mentionWarnings: React$ElementRef<MentionWarnings> = React.createRef(); inputBlurTimeoutId: ?TimeoutID = null; @@ -194,8 +194,8 @@ class ComposeBox extends PureComponent<Props, State> { this.setMessageInputValue(message); if (completionType === '@') { - if (this.mentionWarnings) { - this.mentionWarnings.getWrappedInstance().handleMentionSubscribedCheck(completion); + if (this.mentionWarnings.current) { + this.mentionWarnings.current.getWrappedInstance().handleMentionSubscribedCheck(completion); } } }; @@ -263,8 +263,8 @@ class ComposeBox extends PureComponent<Props, State> { this.setMessageInputValue(''); - if (this.mentionWarnings) { - this.mentionWarnings.getWrappedInstance().clearMentionWarnings(); + if (this.mentionWarnings.current) { + this.mentionWarnings.current.getWrappedInstance().clearMentionWarnings(); } dispatch(sendTypingStop(narrow)); @@ -379,13 +379,7 @@ class ComposeBox extends PureComponent<Props, State> { return ( <View style={this.styles.wrapper}> - <MentionWarnings - narrow={narrow} - stream={stream} - ref={component => { - this.mentionWarnings = component; - }} - /> + <MentionWarnings narrow={narrow} stream={stream} ref={this.mentionWarnings} /> <View style={[this.styles.autocompleteWrapper, { marginBottom: height }]}> <TopicAutocomplete isFocused={isTopicFocused}

Thanks for the diff. I did try this earlier during debugging, but it did not make it to the final version.
Used it now.

chrisbobbe · 2020-07-02T20:59:10Z

src/autocomplete/AutocompleteView.js

@@ -19,14 +19,15 @@ type Props = $ReadOnly<{|
  isFocused: boolean,
  text: string,
  selection: InputSelection,
-  onAutocomplete: (input: string) => void,
+  onAutocomplete: (input: string, completion: string, lastWordPrefix: string) => void,


I think it would be helpful to have some JSDoc about the interface of the AutocompleteView component, in particular, describing what information is represented by completion and lastWordPrefix here. I don't really understand it just from the variable names, and it shouldn't be necessary (if we can avoid it) to have to inspect the implementation.

chrisbobbe · 2020-07-02T21:07:11Z

src/compose/ComposeBox.js

@@ -184,8 +190,14 @@ class ComposeBox extends PureComponent<Props, State> {
    dispatch(draftUpdate(narrow, message));
  };

-  handleMessageAutocomplete = (message: string) => {
+  handleMessageAutocomplete = (message: string, completion: string, completionType: string) => {


Here, we've called the third argument completionType; in AutocompleteView, it looks like we call the same argument lastWordPrefix. I don't see the connection right away, so it's hard to tell if this is a mistake or if these actually correspond to each other.

I think...it's not a mistake, and in both cases we expect them to be the keys of the prefixToComponent lookup object in AutocompleteView.js, i.e., ':' or '#' or '@'.

If that's the case, let's see if we can say that explicitly with the types, so instead of string, it would be ':' | '#' | '@'. A JSDoc would certainly help with this as well, as I've mentioned in another comment. 🙂

I'm also looking at the message argument in of handleMentionSubscribedCheck and the fact that we're passing completion as that argument. I don't currently have a very clear picture of what either one means.

One thing that comes to mind with a name like message is that we do have a Message type. Maybe message could be named more specifically, so that we don't think of the Message type (which I think is irrelevant here as we're talking about a message that hasn't been sent yet)?

Since we've designed MentionWarnings so that handleMentionSubscribedCheck gets called from a ref, from the outside, the interface of handleMentionSubscribedCheck effectively becomes an important part of the interface of MentionWarnings. (Most of the time, these methods would only be called from within the component's implementation, so they aren't part of the component's outward-facing interface.) That would be good information to have in a JSDoc for MentionWarnings. 🙂

If that's the case, let's see if we can say that explicitly with the types, so instead of string, it would be ':' | '#' | '@'. A JSDoc would certainly help with this as well, as I've mentioned in another comment. slightly_smiling_face

I had tried ':' | '#' | '@' earlier as the type, but it causes the Flow error, because the value is derived from some plumbing in getAutocompleteFilter, and Flow is not intelligent enough to figure out that only these three are possible values, instead of a complete string.

agrawal-d · 2020-07-03T10:37:40Z

One other thing: Do you think a cancel button would fit in there somewhere, like the "x" button in the web app? It would just call clearMentionWarnings, I think. People might get frustrated that the warnings are hiding the message list from view, with no way to dismiss them except to send a message or subscribe the user.

It is possible to dismiss the warning by tapping on it. An 'x' button would be even better, but there is no space :'(

Even better would be to automatically dismiss a warning if the user removes the mention of that user from the input text, if that's easy.

I agree, but that would be difficult to implement, and even the webapp does not do this.

chrisbobbe · 2020-07-03T20:12:10Z

It is possible to dismiss the warning by tapping on it. An 'x' button would be even better, but there is no space :'(

Ah OK, got it. Yep, I just tested this on my physical iOS device and it works, though it takes several seconds. I expect this is because the app is slower in development than production, even on a physical device. But I looked at the code and see how this is happening, so, sounds good.

agrawal-d · 2020-07-04T10:38:26Z

Thanks for the review, @chrisbobbe! I've pushed some changes.

chrisbobbe

Thanks, @agrawal-d! A few comments below, then I think this will be ready, unless @gnprice has anything to add.

chrisbobbe · 2020-07-30T22:34:23Z

src/message/MentionedUserNotSubscribed.js

+  },
+});
+
+class MentionedUserNotSubscribed extends Component<Props> {


Is there a reason to make this a Component instead of a PureComponent?

No - I just read about PureComponent in performance.md; will use it now.

chrisbobbe · 2020-07-30T23:13:57Z

src/compose/MentionWarnings.js

+    }
+
+    if (
+      !(await api.getSubscriptionToStream(auth, mentionedUser.user_id, stream.stream_id))


I think we should assume that the Promise returned by api.getSubscriptionToStream might reject, likely because of some kind of failure with the request.

Looks like there's a handy function showErrorAlert we could use. I don't know how precise we should be about the error message; it seems a little silly to give a full paragraph like (jokingly, but still, I don't think it would boil down much): "Oh, well, we were about to go run and check to see if this user was subscribed or not, but something went wrong (details details) and, well, in the end we're not really sure if there'll be a problem if you @-mention the user, so I guess do so at your own risk? 🤷 "

Maybe just something like "Failed to retrieve some details about the @-mentioned user." What do you think?

I am inclined towards silently ignoring the error in this case, because the error will only confuse the user - did the @-mention get sent, or did it not?

the error will only confuse the user - did the @-mention get sent, or did it not?

Well, that uncertainty is pretty appropriate! After all, the point of these warnings is that the @-mention won't reach the mentioned user if they're not subscribed.

We'd like people to have confidence in general about whether someone else is going to get notified about their message or not. In cases where really don't know, the way to maintain that confidence is to say we don't know. Otherwise, when we do know and don't show a warning, people won't know whether to believe it, or whether instead this is another of those times that we failed to show them a warning before the surprising thing happened.

I think a toast would be the appropriate form to report the issue. showErrorAlert pops up a modal dialog box, which is more interruptive than seems appropriate for this condition. Compare:
https://material.io/components/dialogs#usage
https://material.io/components/snackbars#usage
(where "snackbar" is the current Material term for something very similar to a toast.)

Here's another iteration on the text: "Couldn't load information about ${user.full_name}".

Yeah, makes sense.

agrawal-d · 2020-08-04T10:38:32Z

Thanks for the review, @gnprice! I've pushed some changes.

gnprice

One tiny comment below. Otherwise looks good!

I should actually play around with the UI a bit -- then probably merge (fixing the small thing below if you haven't beaten me to it.)

gnprice · 2020-08-06T06:09:08Z

src/compose/MentionWarnings.js

+        id: "Couldn't load information about {fullName}",
+        defaultMessage: "Couldn't load information about {fullName}'",


Suggested change

id: "Couldn't load information about {fullName}",

defaultMessage: "Couldn't load information about {fullName}'",

id: "Couldn't load information about {fullName}",

defaultMessage: "Couldn't load information about {fullName}",

This also serves as a reminder that we should really make our own little wrapper for formatMessage that saves us from having to repeat the string twice 🙂

Creates a binding to the endpoint '/users/{user_id}/subscriptions/{stream_id}' that returns whether or not a user is subscribed to a stream. See https://zulip.com/api/get-subscription-status for details.

gnprice · 2020-08-06T07:40:26Z

I think I want to merge first and then follow up on these, but here are some comments on the UI design.

The "Subscribe" button has an odd shape:

Very tall vertical margins around the text, and quite narrow horizontal margins.

It looks like that ZulipButton has a height of 44px? I believe we'd said elsewhere that the ideal minimum height for a touch target is 48px. So I imagine what's going on is:

the button is that tall to make it an acceptably tall touch target
that causes the tall vertical margins seen around the text
and there isn't any correspondingly large horizontal padding/margin.

I think the ideal visual shape for a button isn't quite so tall. Material specifies 36px height:
https://material.io/components/buttons#specs
But then the touch target can extend into the margin outside the button, beyond the space that's drawn to look like a button.

The border at the top of the banner feels pretty heavy:

and also feels unbalanced, in that there isn't a corresponding border at the bottom.

How about just not having a border? The orange "warning" color should contrast pretty sharply with the message list's background already.

The orange background color doesn't feel great to me, somehow. I appreciate that you have a comment saying something about where it came from:

    backgroundColor: 'hsl(40, 100%, 60%)', // Material warning-color

I'm not finding that in a quick look at https://material.io/design/color/the-color-system.html and some related pages. Where in the Material docs did you find the reference to that color?

Also not finding that specific color (aka #ffbb33) anywhere in the Material palette as represented here:
https://material.io/resources/color/

The orange color is pretty light. Light backgrounds are perfectly fine but it's important to use dark text with them. There's a nice discussion here:
https://material.io/design/color/text-legibility.html

The Material color tool says when I input that color:

White Text: NOT LEGIBLE ⚠️
Black Text: min 59% opacity

Very similar result with the color that's used for the button:

White Text: NOT LEGIBLE ⚠️
Black Text: min 60% opacity

The animation when the banner expands in is kind of odd. It's the exact same one we have for some other banners, so I don't want to emphasize this too much. But it's pretty equally odd for all of them -- it really doesn't make any sense for a banner.

Specifically the animation sequence is:

Start state: no banner.
Suddenly all at once the rest of the layout moves around, to make room for where the banner is going to appear.
In the center of that space where the banner will be, there's a tiny rectangle which is the banner scaled down.
From that center, it expands out to fill the space.
This scaling is happening in all directions -- when it's halfway done expanding, it's half the height it's going to be and also half the width. The rest of the space the banner will fill is just blank space -- we made room for it all at once in step 2.

It'd be good to fix that and do something better. In fact it'd quite likely be an improvement to just drop animation entirely for these banners. (This one; the "N unreads" banner, which is by far the most common; and not sure offhand if other banners like "Connecting..." or "No Internet connection" behave this way too.)

Better yet than no animation would be an appropriate animation. The logical thing for a banner is to slide in from -- in this case the bottom since it's extending up from something, and from the top for the UnreadNotice banner and others that appear from the top. I took a quick look at Material on banners, and it says something similar:

Banners typically appear when a screen loads content.

Banners that appear after a screen loads should animate on screen from the top of a layout. If the banner is at the same elevation as content, it pushes content downwards.

(Material banners always appear at the top.)

Creates a component 'MentionedUserNotSubscribed' that can be used to shows a warning when an @-mentioned user is not subscribed to a stream they are mentioned in, with a button to subscribe them.

Adds more parameters to the callback `onAutocomplete` - namely 'completion' and 'lastWordPrefix', which can be used by the handler to perform more complex processing, if needed.

Creates a component, `MentionWarnings`, that processes and renders warnings when a user that's not subscribed to a stream is mentioned. This is an uncontrolled component, and will be accessed using refs. Therefore, passing ` { withRef: true }` option in the `connect` function is required.

Show a warning when @-mentioning a user who is not subscribed to the stream the user was mentioned in, with an option to subscribe them to the stream. Use the created pipeline in the previous commits to enable the same. Fixes: zulip#3373.

Renames the 'message' parameter in the 'handleMessageAutocomplete' method in 'ComposeBox' to 'completedText' for better clarity, so that we don't think of the 'Message' type.

No need to repeat the name of the component. (I think these names are a remnant of a previous version where these styles lived in a more global location.)

This allows the caller to customize the appearance of the text, where needed e.g. to go along with customizations to the button's background color.

This color is a pretty light one; it needs a dark text color to get acceptable contrast for legibility.

gnprice · 2020-08-06T09:18:27Z

OK, and merged!

Of the UI remarks I made in the previous comment, I went and fixed the color contrast issue, because that's the only one where the issue is really unacceptable -- it's the only one that can make that piece of the app unusable for some people. Also fixed the tiny point mentioned in the previous comment.

Fixing the text contrast required a bit of digging into the ZulipButton component, and that led me to do some other work there. I'll send that separately, as well as some changes addressing the other UI issues.

This animation looks kind of odd, and inappropriate for the entrance of a banner. See description here: zulip#4101 (comment) Ideally we'd have an appropriate animation, like the banner sliding in from the bottom. But no animation at all actually looks pretty OK to me here, and definitely better than this odd animation. (We use the same odd animation for the "N unreads" banner, and it's equally inappropriate there. Likely we should just drop it there, too.)

Now that the `Input` component clearly doesn't use the ref on its child `TextInput` -- only `Input`'s consumers use it; see the previous commit -- we can straightforwardly use the new `React.createRef` API. We can also do so in `SmartUrlInput`. This will smoothen some necessary changes during the upcoming RN v0.61 -> v0.62 upgrade. In particular, it seems that the way `TextInput` is defined has changed in an interesting way (it certainly has changed), or else something happened in the Flow upgrade, to cause this error in several places where we use `TextInput` as a type: ``` Cannot use `TextInput` as a type. A name can be used as a type only if it refers to a type definition, an interface definition, or a class definition. To get the type of a non-class value, use `typeof`. ``` I discovered, in this commit, that `React$Ref`, not `React$ElementRef`, seems to be the correct type for the `ref` attribute and the return value of `React.createRef`. Weeks ago, we used `React$RefElement` for `this.mentionWarnings` in `ComposeBox` and overlooked [1] the fact that it ended up being more or less completely un-typechecked, in 20bd98a. I'll add a TODO in the next commit, saying we should fix that. [1] zulip#4101 (comment)

Prompted by discussion on zulip#4101 [1]. Flow started supporting method calls in optional chains in v0.112.0 [2], but ESLint isn't yet on board [3]; it gives a false positive for `no-unused-expressions`. A comment on the ESLint issue [4] suggests we could be more systematic by using an ESLint plugin from Babel, instead of one-off suppressions of `no-unused-expressions` like this one. That plugin moved from (on NPM) `babel-eslint-plugin` to `@babel/eslint-plugin`. We can't use the new one until we're on ESLint 7 (see zulip#4254), but we could probably use the old one. [1] zulip#4101 (comment) [2] facebook/flow#4303 [3] eslint/eslint#11045 [4] eslint/eslint#11045 (comment)

agrawal-d requested review from gnprice, chrisbobbe and rk-for-zulip May 12, 2020 08:18

agrawal-d force-pushed the unsub branch from c0164a5 to 7bdbe42 Compare May 12, 2020 08:23

agrawal-d added a-compose/send Compose box, autocomplete, camera/upload, outbox, sending P1 high-priority blocked on other work To come back to after another related PR, or some other task. labels May 12, 2020

agrawal-d removed the blocked on other work To come back to after another related PR, or some other task. label Jun 11, 2020

agrawal-d force-pushed the unsub branch 2 times, most recently from fd66ed6 to b1454cb Compare June 12, 2020 10:30

gnprice reviewed Jun 12, 2020

View reviewed changes

agrawal-d removed the request for review from rk-for-zulip June 15, 2020 06:44

agrawal-d force-pushed the unsub branch 2 times, most recently from b722eae to 27e53f3 Compare June 15, 2020 13:04

agrawal-d requested a review from gnprice June 15, 2020 13:06

agrawal-d commented Jun 15, 2020

View reviewed changes

src/compose/MentionWarnings.js Outdated Show resolved Hide resolved

chrisbobbe requested changes Jul 2, 2020

View reviewed changes

agrawal-d force-pushed the unsub branch from 27e53f3 to 2dec8fc Compare July 4, 2020 10:31

agrawal-d requested a review from chrisbobbe July 4, 2020 10:38

agrawal-d force-pushed the unsub branch from 2dec8fc to f044185 Compare July 4, 2020 10:40

chrisbobbe mentioned this pull request Jul 15, 2020

Allow viewing message edit history #4173

Open

chrisbobbe reviewed Jul 31, 2020

View reviewed changes

agrawal-d force-pushed the unsub branch from 9f574fe to 57f1100 Compare August 4, 2020 10:56

gnprice reviewed Aug 6, 2020

View reviewed changes

api: Create binding to get subscription status to a stream.

5844339

Creates a binding to the endpoint '/users/{user_id}/subscriptions/{stream_id}' that returns whether or not a user is subscribed to a stream. See https://zulip.com/api/get-subscription-status for details.

agrawal-d and others added 8 commits August 6, 2020 02:10

compose: Create component to warn on unsubscribed @-mentionee.

de6c66c

Creates a component 'MentionedUserNotSubscribed' that can be used to shows a warning when an @-mentioned user is not subscribed to a stream they are mentioned in, with a button to subscribe them.

autocomplete: Pass more information to callback.

b023922

Adds more parameters to the callback `onAutocomplete` - namely 'completion' and 'lastWordPrefix', which can be used by the handler to perform more complex processing, if needed.

compose [nfc]: Rename a parameter to 'completedText' for accuracy.

0f15268

Renames the 'message' parameter in the 'handleMessageAutocomplete' method in 'ComposeBox' to 'completedText' for better clarity, so that we don't think of the 'Message' type.

compose [nfc]: Shorten names of some styles.

1ea9e9e

No need to repeat the name of the component. (I think these names are a remnant of a previous version where these styles lived in a more global location.)

ZulipButton: Offer a textStyle prop.

387ae00

This allows the caller to customize the appearance of the text, where needed e.g. to go along with customizations to the button's background color.

compose: Fix text-color contrast in mention-unsubscribed warning.

29168ac

This color is a pretty light one; it needs a dark text color to get acceptable contrast for legibility.

gnprice force-pushed the unsub branch from 57f1100 to 29168ac Compare August 6, 2020 09:15

gnprice merged commit 29168ac into zulip:master Aug 6, 2020

This was referenced Aug 6, 2020

UI tweaks on mention-unsubscribed warning #4219

Merged

StyleSheet.create and other refactors #4221

Merged

gnprice mentioned this pull request Aug 11, 2020

Tweak UI of mention-unsubscribed warning #4224

Open

7 tasks

chrisbobbe mentioned this pull request Oct 28, 2020

Decouple Navigation from Redux (Part 2) #4278

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn on @-mentioning someone who won't see it because not subscribed #4101

Warn on @-mentioning someone who won't see it because not subscribed #4101

agrawal-d commented May 12, 2020 •

edited

agrawal-d commented May 12, 2020

agrawal-d commented May 13, 2020

agrawal-d commented Jun 11, 2020 •

edited

agrawal-d commented Jun 12, 2020

gnprice left a comment

gnprice Jun 12, 2020

agrawal-d Jun 15, 2020 •

edited

gnprice Jun 12, 2020

agrawal-d Jun 15, 2020

agrawal-d commented Jun 15, 2020

chrisbobbe left a comment •

edited

chrisbobbe Jul 2, 2020

agrawal-d Jul 4, 2020

chrisbobbe Jul 2, 2020

chrisbobbe Jul 2, 2020

chrisbobbe Jul 2, 2020

agrawal-d Jul 4, 2020

agrawal-d commented Jul 3, 2020 •

edited

chrisbobbe commented Jul 3, 2020

agrawal-d commented Jul 4, 2020 •

edited

chrisbobbe left a comment

chrisbobbe Jul 30, 2020

agrawal-d Jul 31, 2020

chrisbobbe Jul 30, 2020

agrawal-d Jul 31, 2020

gnprice Aug 4, 2020

agrawal-d Aug 4, 2020

agrawal-d commented Aug 4, 2020

gnprice left a comment

gnprice Aug 6, 2020

gnprice commented Aug 6, 2020

gnprice commented Aug 6, 2020

		onAutocomplete: (input: string) => void,
		processAutoComplete: (completion: string, completionType: string) => void,

		id: "Couldn't load information about {fullName}",
		defaultMessage: "Couldn't load information about {fullName}'",

Warn on @-mentioning someone who won't see it because not subscribed #4101

Warn on @-mentioning someone who won't see it because not subscribed #4101

Conversation

agrawal-d commented May 12, 2020 • edited

agrawal-d commented May 12, 2020

agrawal-d commented May 13, 2020

agrawal-d commented Jun 11, 2020 • edited

agrawal-d commented Jun 12, 2020

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d Jun 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d commented Jun 15, 2020

chrisbobbe left a comment • edited

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

agrawal-d commented Jul 3, 2020 • edited

chrisbobbe commented Jul 3, 2020

agrawal-d commented Jul 4, 2020 • edited

chrisbobbe 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

agrawal-d commented Aug 4, 2020

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Aug 6, 2020

gnprice commented Aug 6, 2020

agrawal-d commented May 12, 2020 •

edited

agrawal-d commented Jun 11, 2020 •

edited

agrawal-d Jun 15, 2020 •

edited

chrisbobbe left a comment •

edited

agrawal-d commented Jul 3, 2020 •

edited

agrawal-d commented Jul 4, 2020 •

edited