Page MenuHomePhabricator
Differential D9095

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

Authored by inka on Sep 7 2023, 1:35 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 21, 6:08 AM
Unknown Object (File)
Thu, Nov 21, 6:08 AM
Unknown Object (File)
Thu, Nov 21, 6:08 AM
Unknown Object (File)
Thu, Nov 21, 6:08 AM
Unknown Object (File)
Thu, Nov 21, 6:08 AM
Unknown Object (File)
Thu, Nov 21, 6:08 AM
Unknown Object (File)
Thu, Nov 21, 6:07 AM
Unknown Object (File)
Wed, Nov 20, 5:48 PM
View All 97 Files
Subscribers
ashoat

Details

Reviewers
rohan
ginsu
atul
tomek
Commits
rCOMMa37290eb224f: [native] Block editing messages on search and pinned messages screens, and of…
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
Branch
inka/edit_fix
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka created this revision.Sep 7 2023, 1:35 AM
Herald added a subscriber: ashoat. · View Herald TranscriptSep 7 2023, 1:35 AM
inka edited the summary of this revision. (Show Details)Sep 7 2023, 1:36 AM
inka edited the summary of this revision. (Show Details)Sep 7 2023, 1:45 AM
inka edited the test plan for this revision. (Show Details)
Harbormaster completed remote builds in B22421: Diff 30825.Sep 7 2023, 1:54 AM
inka requested review of this revision.Sep 7 2023, 1:54 AM
tomek added inline comments.Sep 7 2023, 2:34 AM
native/chat/text-message-tooltip-modal.react.js
134–135

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
inka updated this revision to Diff 31308.Sep 20 2023, 3:48 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.

Harbormaster completed remote builds in B22756: Diff 31308.Sep 20 2023, 4:06 AM
tomek accepted this revision.Sep 20 2023, 5:42 AM
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 accepted this revision.Sep 25 2023, 6:35 AM
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)

atul accepted this revision.Sep 26 2023, 2:06 PM
This revision is now accepted and ready to land.Sep 26 2023, 2:06 PM
inka updated this revision to Diff 31598.Oct 2 2023, 7:56 AM

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
Harbormaster completed remote builds in B22938: Diff 31598.Oct 2 2023, 8:15 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
inka updated this revision to Diff 31614.Oct 3 2023, 7:02 AM

Address review

tomek accepted this revision.Oct 3 2023, 7:18 AM
This revision is now accepted and ready to land.Oct 3 2023, 7:18 AM
Harbormaster completed remote builds in B22951: Diff 31614.Oct 3 2023, 7:20 AM
Closed by commit rCOMMa37290eb224f: [native] Block editing messages on search and pinned messages screens, and of… (authored by inka). · Explain WhyOct 4 2023, 2:22 AM
This revision was automatically updated to reflect the committed changes.
inka added a commit: rCOMMa37290eb224f: [native] Block editing messages on search and pinned messages screens, and of….

Revision Contents
Changeset List

PathSize
native/
chat/
27 lines

Diff 30825

View Options

native/chat/text-message-tooltip-modal.react.js

Loading...