pr-topic-edit-modal #5510

dootMaster · 2022-09-29T08:30:50Z

Revised draft PR for #5365.

chrisbobbe · 2022-09-29T10:51:38Z

Great, thanks! I believe your previous revision was #5505. For the future, we prefer updating an existing PR by force-pushing to the same PR branch, rather than starting a new PR. Starting a new PR makes it harder to find relevant discussion on earlier iterations of the work. 🙂

I hope to get another review for you soon; gotta run for dinner now. 🙂

src/common/ZulipTextButton.js

chrisbobbe

Thanks for the review, this is getting closer! 🙂 Comments below.

src/topics/TopicEditModal.js

src/boot/TopicEditModalProvider.js

src/topics/TopicEditModal.js

src/common/ZulipTextButton.js

src/topics/TopicEditModal.js

chrisbobbe · 2022-10-03T10:25:29Z

src/topics/TopicEditModal.js

+  });
+
+  const handleSubmit = async () => {
+    if (topicName === '') {


This topicName === '' expression appears here and also where we pass the disabled prop to a ZulipTextButton. Let's pull it out into a const validationErrors = … directly in the body of TopicEditModal, like we do in ComposeBox.

Then, let's make it a bit more sophisticated: topicName.trim() === '' (or equivalently, topicName.trim().length === 0). When the server rejects '' as a topic, it also rejects ' ', etc., so we should too, to avoid sending a request we know the server will reject.

Hmm, looking again at ComposeBox, I remember that sometimes servers do accept an empty topic: in particular, if the realm doesn't require topics in stream messages. So you could follow ComposeBox by adding a

const mandatoryTopics = useSelector(state => getRealm(state).mandatoryTopics);

and using mandatoryTopics in the new const validationErrors. So, specifically, if topicName.trim() === '' and mandatoryTopics, then say the input is invalid because topics are required and no topic was given.

You could also have validationErrors include an item for "the topic is too long," though I see you've handled that already with the maxLength prop on the input component, which seems OK. I slightly prefer doing that here for consistency, but could go either way.

I've implemented validationErrors. Please take a look at error message wording. 📝

dootMaster · 2022-10-06T18:59:07Z

@chrisbobbe What do you think about streamId initialized as -1 in TopicEditModalProvider?

This is the error when I type the props to accept streamId: number | null:

Cannot call fetchSomeMessageIdForConversation with streamId bound to streamId because null [1] is incompatible with number [2].Flow(incompatible-call)

I didn't want to alter fetchSomeMessageIdForConversation. 😨

dootMaster · 2022-10-06T23:45:11Z

This commit no longer uses disabled or isPressHandledWhenDisabled in ZulipTextButton, since instead of disabling the button, we provide the user an error alert explaining what could be wrong with their input and return out. It's likely still a useful thing to have in the component, but this no longer addresses the specific issue mentioned in #M5365. Should we open a separate issue and update the component?

@gnprice @chrisbobbe

gnprice

Thanks @dootMaster, and @chrisbobbe for the previous reviews! I've just taken a look through this, and have code comments below.

Would you also post screenshots of what the current version looks like? That'd be helpful to let people review the design and UX.

src/common/ZulipTextButton.js

gnprice · 2022-10-07T21:18:47Z

src/common/ZulipTextButton.js

+   * isPressHandledWhenDisabled - Whether `onPress` is used even when
+   *   `disabled` is true.
+   */
+  isPressHandledWhenDisabled?: boolean,


nit: don't repeat the name in the jsdoc -- the name is already visible just below it

gnprice · 2022-10-07T21:20:33Z

src/common/ZulipTextButton.js

+    onPress,
+    disabled = false,
+    isPressHandledWhenDisabled = false,
+  } = props;


nit: this and some other formatting changes in commit 3, which otherwise doesn't touch this code which was added in commit 1. Instead, the commit that adds the code should give it the appropriate formatting in the first place.

Simplest way to fix this is probably to use git rebase -i to edit the first commit; then auto-format this file while there.

gnprice · 2022-10-07T21:35:31Z

src/boot/TopicEditModalProvider.js

+  const [topicModalProviderState, setTopicModalProviderState] = useState({
+    visible: false,
+    streamId: -1,
+    oldTopic: '',
+  });


Generally we avoid having things float around with bogus values, like a "stream ID" that isn't actually the ID of any stream. That's because if we ever actually consult such a value and try to use it, that's likely to be a bug. So if we don't want to ever actually consult the value… best to write the data structures, and the types, so that we don't have the value lying around in the first place.

This is the error when I type the props to accept streamId: number | null:

Cannot call fetchSomeMessageIdForConversation with streamId bound to streamId because null [1] is incompatible with number [2].Flow(incompatible-call)

I didn't want to alter fetchSomeMessageIdForConversation. 😨

Indeed, we don't want to pass null as a stream ID to that function, and we should keep the types saying we don't do so.

We also don't want to pass it a bogus value like -1. So you should read this type error as telling you about a problem that's already there in the version with -1, and that the type-checker just isn't able to highlight for us in that version because using a bogus value -1 that has the same type as the legitimate values makes the issue invisible. Using null instead makes it visible to the type-checker where the lack of a real value can and can't flow, which is an improvement.

An adequate solution would be to say streamId: null | number (and similarly for the topic), and then add appropriate control flow so we don't attempt things like fetchSomeMessageIdForConversation when we don't actually have a stream and topic.

A further improvement would be to unify together the different states of visible, streamId, and oldTopic. It's always the case that when visible is true, we have an actual stream ID and topic, and when it's false, we don't. So ideally we'd express that in the state value's type, and then we need a smaller number of conditionals.

State unified!

Happy to learn about control flow. I'm not sure if I performed error handling correctly for the submit handler, but it now contains control flow that checks stream ID and topic for null.

gnprice · 2022-10-07T21:37:42Z

src/topics/TopicEditModal.js

+    if (validationErrors.length > 0) {
+      const errorMessages = validationErrors
+        .map(error => {
+          // 'mandatory-topic-empty' | 'user-did-not-edit' | 'topic-too-long'


nit: this comment doesn't seem to add anything not expressed in the case lines below; better to cut it

The handleSubmit in ComposeBox contains a similar comment. Would you like me to mark it for removal?

const handleSubmit = useCallback(() => { //... if (validationErrors.length > 0) { const msg = validationErrors .map(error => { // 'upload-in-progress' | 'message-empty' | 'mandatory-topic-empty' switch (error) { //...

Hmm, indeed! Just pushed a quick commit deleting that, and crediting you: bfc0fb6

src/topics/TopicEditModal.js

gnprice · 2022-10-07T21:53:53Z

src/topics/TopicEditModal.js

+  const modalStyles = createStyleSheet({
+    backdrop: {
+      position: 'absolute',
+      backgroundColor: 'black',
+      opacity: 0.25,
+      top: 0,
+      left: 0,
+      right: 0,
+      bottom: 0,
+    },
+    wrapper: {
+      flex: 1,
+      justifyContent: 'center',
+      alignItems: 'center',
+    },
+    modal: {
+      justifyContent: 'flex-start',
+      backgroundColor,
+      padding: 15,
+      shadowOpacity: 0.5,
+      shadowColor: 'gray',
+      shadowOffset: {
+        height: 5,
+        width: 5,
+      },
+      shadowRadius: 5,
+      borderRadius: 5,
+      width: '90%',
+    },
+    buttonContainer: {
+      flexDirection: 'row',
+      justifyContent: 'flex-end',
+    },
+    titleText: {
+      fontSize: 18,
+      lineHeight: 21,
+      color: BRAND_COLOR,
+      marginBottom: 10,
+      fontWeight: 'bold',


The details of this style -- do they come from a particular style guide or design spec, or are they based on tweaking them until they looked about right?

I'm thinking in particular of:

the backdrop as 25% black

the modal's shadow

the modal's width, padding, and corners

the title's text color and other details

The backdrop as 25% black is from me trying to match the action sheet background when it's toggled.

The modal's shadow is from eyeballing, but I see now Popup.js has values I can borrow.

I've looked at the Material Design specs for dialogue and made some changes to the width and text color. The border radius is from CSS from ComposeBox.js.

src/topics/TopicEditModal.js

dootMaster · 2022-10-10T21:31:56Z

Here's some screenshots of the latest styling.

New shadow values pulled from Popup.js. New 280 pixel width from Material Design specs.

dootMaster · 2022-10-10T21:49:36Z

I'd like to direct attention at line 114 in handleSubmit in TopicEditModal.js. Let me know if this error handling is usable:

if (streamId !== null && oldTopic !== null && newTopicInputValue !== null) {
  try {
    const messageId = await fetchSomeMessageIdForConversation(
    // ...
} else {
  throw new Error(_('Topic, streamId, or input value null.'));
}

dootMaster · 2022-10-10T21:57:13Z

New ✨push✨.

@chrisbobbe @gnprice

and bumping some questions:

The handleSubmit in ComposeBox contains a similar comment. Would you like me to mark it for removal?

const handleSubmit = useCallback(() => {
//...
   if (validationErrors.length > 0) {
     const msg = validationErrors
       .map(error => {
         // 'upload-in-progress' | 'message-empty' | 'mandatory-topic-empty'
         switch (error) {
//...

and

This update no longer uses disabled or isPressHandledWhenDisabled in ZulipTextButton, since instead of disabling the button, we provide the user an error alert explaining what could be wrong with their input and return out. It's likely still a useful thing to have in the component, but this no longer addresses the specific issue mentioned in #M5365. Should we open a separate issue and update the component?

Flow already knows that these are the only three possible values for this variable: if you delete or munge one of the cases, you get a type error at the `ensureUnreachable` call. Thanks to Leslie Ngo for spotting this: #5510 (comment)

gnprice

Thanks @dootMaster for the revision, and for all your work on this so far!

Sorry for the delay in review -- I've been catching up on things after having been on vacation and then sick with covid. Comments below. I think this is getting pretty close to merge!

I see you've closed the PR. If you're no longer up for taking this code the rest of the way there, that's OK too; let us know.

gnprice · 2022-10-21T22:00:14Z

src/topics/TopicEditModal.js

+    backdrop: {
+      position: 'absolute',
+      backgroundColor: 'black',
+      opacity: 0.25,


Continuing from #5510 (comment) :

The backdrop as 25% black is from me trying to match the action sheet background when it's toggled.

Cool, that makes sense.

Let's have a one- or two-line comment here explaining that that's the reasoning for these choices of opacity and backgroundColor.

gnprice · 2022-10-21T22:02:03Z

src/topics/TopicEditModal.js

+      shadowOpacity: 0.5,
+      elevation: 8,
+      shadowRadius: 16,
+      borderRadius: 5,


Similarly, let's have a comment saying that these are copied from Popup.js.

(Better yet might be to do some refactoring so that these details can live in one place and be shared. But that's fine to leave out of scope for this PR.)

gnprice · 2022-10-22T04:41:44Z

src/topics/TopicEditModal.js

+    if (streamId !== null && oldTopic !== null && newTopicInputValue !== null) {
+      try {
+        const messageId = await fetchSomeMessageIdForConversation(
+          auth,
+          streamId,
+          oldTopic,
+          streamsById,
+          zulipFeatureLevel,
+        );
+        if (messageId == null) {
+          throw new Error(
+            _('No messages in topic: {streamAndTopic}', {
+              streamAndTopic: `#${streamsById.get(streamId)?.name ?? 'unknown'} > ${oldTopic}`,
+            }),
+          );
+        }
+        await api.updateMessage(auth, messageId, {
+          propagate_mode: 'change_all',
+          subject: newTopicInputValue.trim(),
+          ...(zulipFeatureLevel >= 9 && {
+            send_notification_to_old_thread: true,
+            send_notification_to_new_thread: true,
+          }),
+        });
+      } catch (error) {
+        showErrorAlert('Failed to edit topic.');
+      } finally {
+        closeEditTopicModal();
+      }
+    } else {
+      throw new Error(_('Topic, streamId, or input value null.'));
+    }


nit: for this sort of error-handling control flow, put the handling right after the condition. Like this:

if (streamId !== null && oldTopic !== null && newTopicInputValue !== null) { throw new Error(_('Topic, streamId, or input value null.')); }

Then the rest of the code can continue onward. Here's a previous discussion of a similar case:
#3888 (comment)

(I guess this is a reply to a question you asked above: #5510 (comment))

gnprice · 2022-10-22T04:57:51Z

src/boot/TopicEditModalProvider.js

+  const startEditTopic = useCallback(
+    (streamIdArg, oldTopicArg) => {
+      const { streamId, oldTopic } = topicModalProviderState;
+      if (streamId === null && oldTopic === null) {


This conditional is effectively meant to ask "is the modal currently inactive/hidden". But it disagrees with some conditionals in TopicEditModal about exactly what that condition should be:

this makes it inactive just if both streamId and oldTopic are null

but those make it inactive just if either of those is null.

That means that if we somehow get into a state where just one of those is null, then these different parts of the code will behave inconsistently: the modal won't actually be shown (the visible prop on Modal will be false), but if the user tries to edit a topic nothing will happen (because this conditional will effectively believe the modal is still visible.)

I believe in this version of the code there's no way to enter that state. But there's nothing particularly to prevent that from becoming possible with some future change to the code. That makes it an example of a "latent bug".

One fix would be to make this an explicit invariant: add an assertion somewhere, either as an if and throw or with the handy invariant function:
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#invariant-assertions

But there's actually a cleaner solution available. As a sort of followup to #5510 (comment) with eliminating bogus values and unifying visible with the other two pieces of this data, we can go another step further and adjust the type so that it excludes the should-be-impossible case of having one null property and one non-null. For the state type, we can replace this:

{ oldTopic: null | string, streamId: null | number, }

with this:

null | { oldTopic: string, streamId: number }

I think a number of spots in the code will get to be a bit cleaner that way.

gnprice · 2022-10-22T05:01:00Z

src/topics/TopicEditModal.js

+          <Input
+            style={styles.marginBottom}
+            value={newTopicInputValue}
+            placeholder="You may need to enter a topic in this stream."


Seeing this in the UI, it feels kind of wordy. It also can overflow the input:

Let's go for just "Topic". It is a placeholder, not an instruction, so it doesn't need to say a lot.

gnprice · 2022-10-22T05:08:45Z

src/common/ZulipTextButton.js

+  /**
+   * True just if the button is in the disabled state.
+   *   https://material.io/design/interaction/states.html#disabled
+   */
+  disabled?: boolean,
+
+  /**
+   * Whether `onPress` is used even when
+   *   `disabled` is true.
+   */
+  isPressHandledWhenDisabled?: boolean,


formatting nit: give each jsdoc a summary line, as its own paragraph. Like this:

/** * True just if the button is in the disabled state. - * https://material.io/design/interaction/states.html#disabled + * + * https://material.io/design/interaction/states.html#disabled */ disabled?: boolean, /** - * Whether `onPress` is used even when - * `disabled` is true. + * Whether `onPress` is used even when `disabled` is true. */ isPressHandledWhenDisabled?: boolean,

dootMaster · 2022-10-23T11:40:11Z

Thanks @dootMaster for the revision, and for all your work on this so far!

Sorry for the delay in review -- I've been catching up on things after having been on vacation and then sick with covid. Comments below. I think this is getting pretty close to merge!

I see you've closed the PR. If you're no longer up for taking this code the rest of the way there, that's OK too; let us know.

Hey yeah I didn't realize this was getting close to merge. I can reopen it and can finish it up.

dootMaster · 2022-11-04T03:56:34Z

@gnprice

Line 36 in TopicEditModal.js, let me know what you think. Not sure if this was handled correctly.

Also had to add typeof control flow at line 47 to make sure newTopicValue is string, but feels like a latent bug.

dootMaster · 2022-11-05T08:55:59Z

ok hold on greg gave me a new git tool, I'll have a proper/clean push soon

dootMaster · 2022-11-08T06:51:50Z

I'm also going to remove the commit that modifies ZulipTextButton to receive a disabled prop. The topic edit feature patch no long makes use of this addition. If we'd still like to add it, I think it probably belongs in it's own PR.

Fixes: zulip#5365

dootMaster · 2022-11-08T23:25:32Z

@gnprice 👀

Line 36 in TopicEditModal.js, let me know what you think. Not sure if this was handled correctly.

Also had to add typeof control flow at line 47 to make sure newTopicValue is string, but feels like a latent bug.

dootMaster force-pushed the pr-topic-edit-modal branch 2 times, most recently from a100423 to 52ad6a3 Compare September 29, 2022 20:49

dootMaster commented Sep 29, 2022

View reviewed changes

src/common/ZulipTextButton.js Outdated Show resolved Hide resolved

chrisbobbe reviewed Oct 3, 2022

View reviewed changes

dootMaster force-pushed the pr-topic-edit-modal branch 5 times, most recently from 0a8d910 to de271d7 Compare October 6, 2022 23:20

dootMaster requested a review from chrisbobbe October 6, 2022 23:28

chrisbobbe mentioned this pull request Oct 7, 2022

Topic edit modal #5505

Closed

gnprice reviewed Oct 7, 2022

View reviewed changes

dootMaster force-pushed the pr-topic-edit-modal branch from de271d7 to 8f75c5e Compare October 10, 2022 21:41

dootMaster requested review from gnprice and chrisbobbe and removed request for chrisbobbe and gnprice October 10, 2022 21:55

dootMaster closed this Oct 20, 2022

gnprice reviewed Oct 22, 2022

View reviewed changes

dootMaster reopened this Oct 23, 2022

dootMaster force-pushed the pr-topic-edit-modal branch 5 times, most recently from d47943e to 75bd9cf Compare November 4, 2022 03:46

dootMaster force-pushed the pr-topic-edit-modal branch from 75bd9cf to 8f75c5e Compare November 8, 2022 06:52

topic edit modal: Add new edit-topic UI.

6bdb7bf

Fixes: zulip#5365

dootMaster force-pushed the pr-topic-edit-modal branch 2 times, most recently from 8f75c5e to 6bdb7bf Compare November 8, 2022 07:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pr-topic-edit-modal #5510

pr-topic-edit-modal #5510

dootMaster commented Sep 29, 2022 •

edited by gnprice

chrisbobbe commented Sep 29, 2022

chrisbobbe left a comment

chrisbobbe Oct 3, 2022

chrisbobbe Oct 3, 2022

dootMaster Oct 6, 2022

dootMaster commented Oct 6, 2022 •

edited

dootMaster commented Oct 6, 2022 •

edited

gnprice left a comment

gnprice Oct 7, 2022

gnprice Oct 7, 2022

gnprice Oct 7, 2022

dootMaster Oct 10, 2022

gnprice Oct 7, 2022 •

edited

dootMaster Oct 10, 2022

gnprice Oct 21, 2022

gnprice Oct 7, 2022

dootMaster Oct 10, 2022

dootMaster commented Oct 10, 2022 •

edited

dootMaster commented Oct 10, 2022 •

edited

dootMaster commented Oct 10, 2022 •

edited

gnprice left a comment

gnprice Oct 21, 2022

gnprice Oct 21, 2022

gnprice Oct 22, 2022

gnprice Oct 22, 2022

gnprice Oct 22, 2022

gnprice Oct 22, 2022

dootMaster commented Oct 23, 2022

dootMaster commented Nov 4, 2022

dootMaster commented Nov 5, 2022

dootMaster commented Nov 8, 2022

dootMaster commented Nov 8, 2022

pr-topic-edit-modal #5510

Are you sure you want to change the base?

pr-topic-edit-modal #5510

Conversation

dootMaster commented Sep 29, 2022 • edited by gnprice

chrisbobbe commented Sep 29, 2022

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

dootMaster commented Oct 6, 2022 • edited

dootMaster commented Oct 6, 2022 • edited

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 Oct 7, 2022 • 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

dootMaster commented Oct 10, 2022 • edited

dootMaster commented Oct 10, 2022 • edited

dootMaster commented Oct 10, 2022 • edited

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

dootMaster commented Oct 23, 2022

dootMaster commented Nov 4, 2022

dootMaster commented Nov 5, 2022

dootMaster commented Nov 8, 2022

dootMaster commented Nov 8, 2022

dootMaster commented Sep 29, 2022 •

edited by gnprice

dootMaster commented Oct 6, 2022 •

edited

dootMaster commented Oct 6, 2022 •

edited

gnprice Oct 7, 2022 •

edited

dootMaster commented Oct 10, 2022 •

edited

dootMaster commented Oct 10, 2022 •

edited

dootMaster commented Oct 10, 2022 •

edited