Page MenuHomePhabricator

[native] Block editing messages on search and pinned messages screens, and of the first message in a thread
ClosedPublic

Authored by inka on Sep 7 2023, 1:35 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jul 16, 9:27 PM
Unknown Object (File)
Fri, Jul 12, 9:26 AM
Unknown Object (File)
Fri, Jul 12, 9:26 AM
Unknown Object (File)
Fri, Jul 12, 9:26 AM
Unknown Object (File)
Fri, Jul 12, 9:26 AM
Unknown Object (File)
Fri, Jul 12, 9:26 AM
Unknown Object (File)
Fri, Jul 12, 9:26 AM
Unknown Object (File)
Fri, Jul 12, 9:24 AM
Subscribers

Details

Summary

issues: https://linear.app/comm/issue/ENG-4834/editing-sidebar-source-message-behaves-weirdly#comment-e38584c8
https://linear.app/comm/issue/ENG-4847/not-possible-to-pin-the-source-messge#comment-9971bb16
This is a bit of a hacky solution to prevent editing in search, pinned messages and of the first message in a thread. The first two cases will be resolved by https://linear.app/comm/issue/ENG-3172/implement-navigating-to-the-message-picked-from-the-search-screen, where we will implement navigating to a message. So that hack will be reverted. The third one looks like a bug, and we will probably want to investigate it at some point, and fix it.

On web everything works correctly, so I left it for now

Test Plan

checked that "edit" is not present in tooltip in search results, pinned messages screen and for the first message in a thread. Checked that it is present for a message in parent thread from which the thread was created, as well as other messages in the thread and parent chat.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka edited the test plan for this revision. (Show Details)
inka requested review of this revision.Sep 7 2023, 1:54 AM
native/chat/text-message-tooltip-modal.react.js
134–135 ↗(On Diff #30825)

This is really hacky. At least, we should use a constant defined somewhere, e.g. MessageSearchRouteName.

Ideally, a screen should tell the tooltip if the message should be editable. E.g. as a parameter to createTooltip function, or through TooltipContext... do you think we can do something like this?

atul requested changes to this revision.Sep 14 2023, 9:19 AM

Requesting changes for feedback above

This revision now requires changes to proceed.Sep 14 2023, 9:19 AM

Use constant. Move logic to ConnectedTextMessage, where other conditions for whether a message can be edited are checked, and a canEditMessage value used to create visibleEntryIDs which are used by TooltipContext to determine whether the item should be rendered, is created.

tomek added inline comments.
native/chat/text-message.react.js
276 ↗(On Diff #31308)

How about renaming it?

281 ↗(On Diff #31308)

What do you think about checking screen name inside this hook? (should be possible using nav hooks)

rohan added inline comments.
native/chat/text-message.react.js
281 ↗(On Diff #31308)

I agree, would make it easier to have the checks in the hook to modify it in the future if needed (and if the hook is used in other places)

This revision is now accepted and ready to land.Sep 26 2023, 2:06 PM

Move logic to a hook. Had to add a hook, because navigation is native specific, and we want this behaviour to only be present on native

inka requested review of this revision.Oct 2 2023, 7:57 AM
tomek requested changes to this revision.Oct 3 2023, 2:20 AM
tomek added inline comments.
native/chat/text-message.react.js
275–276 ↗(On Diff #31598)

Why do we use the same value twice?

native/navigation/nav-selectors.js
416 ↗(On Diff #31598)

Should we use >=?

This revision now requires changes to proceed.Oct 3 2023, 2:20 AM
This revision is now accepted and ready to land.Oct 3 2023, 7:18 AM