Page MenuHomePhabricator

[native] Displaying edit mode
ClosedPublic

Authored by kuba on Apr 7 2023, 6:56 AM.
Tags
None
Referenced Files
F3183886: D7344.id25664.diff
Fri, Nov 8, 10:01 AM
F3183882: D7344.id25092.diff
Fri, Nov 8, 10:01 AM
F3183880: D7344.id25091.diff
Fri, Nov 8, 10:01 AM
F3183877: D7344.id25032.diff
Fri, Nov 8, 10:01 AM
F3183873: D7344.id24961.diff
Fri, Nov 8, 10:01 AM
F3183870: D7344.id.diff
Fri, Nov 8, 10:01 AM
F3183865: D7344.diff
Fri, Nov 8, 10:00 AM
Unknown Object (File)
Tue, Nov 5, 2:38 AM
Subscribers

Details

Summary

Added displaying edit GUI when in edit mode.

Test Plan

Checked if after clicking 'Edit' on the target message edit mode is turned on.
Checked if the displayed content of the edited message work in different situations:

  • if the original message is already edited,
  • if the original message has some markdown,
  • if the original message is really long/multiline,
  • if the original message has replies/quotes,

Screenshot 2023-04-07 at 16.12.23.png (308×770 px, 83 KB)

Screenshot 2023-04-07 at 16.13.04.png (276×754 px, 65 KB)

Screenshot 2023-04-07 at 16.13.38.png (370×774 px, 87 KB)

Screenshot 2023-04-07 at 16.14.31.png (368×764 px, 109 KB)

With already edited content:

Screenshot 2023-04-07 at 16.15.06.png (326×742 px, 59 KB)

On Android:

Screenshot 2023-04-18 at 12.34.36.png (492×664 px, 87 KB)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kuba edited the test plan for this revision. (Show Details)

I love the design of this!

native/chat/chat-input-bar.react.js
550 ↗(On Diff #24785)

is this necessary?

779 ↗(On Diff #24785)

Is this necessary? Up to you, but I would just inline it below. It doesn't seem it's used anywhere else?

kuba marked 2 inline comments as done.

Removed some unnecessary lines

tomek requested changes to this revision.Apr 12 2023, 1:49 AM
tomek added inline comments.
lib/shared/edit-messages-utils.js
95–102 ↗(On Diff #24961)

Wondering about the performance here. The function that is returned from this hook is currently being called as a selector, so potentially it might be often, and we're iterating all the messages of a thread, which might be time consuming.

Do you have some ideas how we can improve the performance here? Maybe we can move something to useMemo call?

native/chat/chat-input-bar.react.js
1019 ↗(On Diff #24961)

This call is really misleading. An argument of useSelector should be a selector - a pure function that takes state and returns something. In this case we're calling a hook as a parameter to another hook - this works, but might cause maintainability issues in the future. It's safer to always call hooks in separate lines.

To fix the issue you can e.g. move useSelector call into useGetEditedMessage.

This revision now requires changes to proceed.Apr 12 2023, 1:49 AM
tomek requested changes to this revision.Apr 13 2023, 2:28 AM
tomek added inline comments.
lib/shared/edit-messages-utils.js
91–114

I'm wondering if we really need this logic. In chat-selectors we're replacing message text with edited content and return ChatMessageItem - can we somehow use that result here?

114

We should make dependencies as concrete as possible to reduce the number of recomputations.

native/chat/chat-input-bar.react.js
545

Can we find a different name? e.g. editedMessage?

550

Do we have to use animated view?

1048

Maybe editedMessagePreview?

This revision now requires changes to proceed.Apr 13 2023, 2:28 AM
kuba marked 3 inline comments as done.

Addressed review comments

Removed redundant useGetEditedMessage function

Fix typos

lib/shared/edit-messages-utils.js
91–114

Really good point. Thanks for pointing this out!

Changed edit state: https://phab.comm.dev/D7414

Now, in the edit state, I store the whole messageInfo instead of just it's id, and then look it's content up again.

native/chat/chat-input-bar.react.js
550

I used it to have an animation of sliding up the edit view. However, it doesn't work when hiding the component. I can remove it if we want consistent behavior.

tomek added inline comments.
native/chat/chat-input-bar.react.js
40–45 ↗(On Diff #25092)

These can be merged

This revision is now accepted and ready to land.Apr 14 2023, 3:21 AM
kuba marked an inline comment as done.

Merge imports