Page MenuHomePhabricator
Differential D5764

[native] Render second TextMessage with all Texts pressable when markdownHasPressable
ClosedPublic
Actions

Authored by ashoat on Nov 29 2022, 8:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 4:16 PM
Unknown Object (File)
Fri, Nov 8, 4:16 PM
Unknown Object (File)
Fri, Nov 8, 4:08 PM
Unknown Object (File)
Fri, Nov 8, 3:59 PM
Unknown Object (File)
Mon, Nov 4, 3:10 AM
Unknown Object (File)
Mon, Nov 4, 3:10 AM
Unknown Object (File)
Sep 28 2024, 3:13 AM
Unknown Object (File)
Sep 27 2024, 7:22 PM
View All 97 Files
Subscribers
tomek

Details

Reviewers
rohan
atul
ginsu
michal
Commits
rCOMMa929cfa91fc3: [native] Render second TextMessage with all Texts pressable when…
Summary

This is a potential solution to the issues we have with Markdown pressables.

Test Plan

I tested this on iOS and Android while pressing all over a message, before and after opening the modal. See attached video

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ashoat created this revision.Nov 29 2022, 8:10 PM
Herald added a subscriber: tomek. · View Herald TranscriptNov 29 2022, 8:10 PM
ashoat added a child revision: D5765: [native] Normalize Android MarkdownLink behavior to match iOS.Nov 29 2022, 8:11 PM

ashoat attached a referenced file: F265588: Screen Recording 2022-11-29 at 11.02.13 PM.mov. (Show Details)Nov 29 2022, 8:12 PM
ashoat added a parent revision: D5763: [native] Pass TextMessage.onPress down via context.
Harbormaster completed remote builds in B13862: Diff 18986.Nov 29 2022, 8:24 PM
ashoat requested review of this revision.Nov 29 2022, 8:24 PM
rohan accepted this revision.Nov 30 2022, 9:40 AM

The comment for secondMessage makes sense, locally I didn't see any issues with the secondMessage being rendered with both existing and new messages being sent.

This revision is now accepted and ready to land.Nov 30 2022, 9:40 AM
rohan added a comment.Nov 30 2022, 11:02 AM

Follow up question, is this message press experience the one we want to keep going forward?

Before when pressing on a message outside the text:

Screenshot 2022-11-30 at 1.55.30 PM.png (1×803 px, 335 KB)

Now:

Screenshot 2022-11-30 at 1.56.09 PM.png (1×798 px, 342 KB)

rohan attached a referenced file: F266973: Screenshot 2022-11-30 at 1.56.09 PM.png. (Show Details)Nov 30 2022, 11:03 AM
rohan attached a referenced file: F266971: Screenshot 2022-11-30 at 1.55.30 PM.png. (Show Details)
Closed by commit rCOMMa929cfa91fc3: [native] Render second TextMessage with all Texts pressable when… (authored by ashoat). · Explain WhyNov 30 2022, 11:10 AM
This revision was automatically updated to reflect the committed changes.
ashoat added a commit: rCOMMa929cfa91fc3: [native] Render second TextMessage with all Texts pressable when….
ashoat added a comment.Nov 30 2022, 2:53 PM

Follow up question, is this message press experience the one we want to keep going forward?

Before when pressing on a message outside the text:

That's the downside of this approach, and that's why we're only doing it for pressables. It's definitely worse

Revision Contents
Changeset List

PathSize
native/
chat/
25 lines
markdown/
29 lines
5 lines

Diff 19026

View Options

native/chat/inner-text-message.react.js

Loading...
View Options

native/markdown/markdown-paragraph.react.js

Loading...
View Options

native/markdown/rules.react.js

Loading...