Page MenuHomePhabricator
Differential D10389

[lib/web/native] Bold ENS names in user mentions
ClosedPublic
Actions

Authored by rohan on Dec 18 2023, 12:04 PM.
Tags
None
Referenced Files
F3307344: D10389.diff
Tue, Nov 19, 1:59 AM
Unknown Object (File)
Fri, Nov 8, 10:03 PM
Unknown Object (File)
Fri, Nov 8, 7:17 PM
Unknown Object (File)
Fri, Nov 8, 7:17 PM
Unknown Object (File)
Fri, Nov 8, 7:17 PM
Unknown Object (File)
Fri, Nov 8, 7:17 PM
Unknown Object (File)
Fri, Nov 8, 7:17 PM
Unknown Object (File)
Fri, Nov 8, 7:17 PM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
atul
ginsu
inka
Commits
rCOMM87417b194198: [lib/web/native] Bold ENS names in user mentions
Summary

Now that the regex in D10388 (previous diff) can support ENS names in @ mentions, we need to make some modifications to createMemberMapForUserMentions since it constructs a member map that is used to bold mentions.

For example, since the map will link a username (john) to a user ID (92019), when we type @john, we associate it to the user ID.

For ENS names, we want to do the same. We want to have both the wallet address and the resolved ENS name mapping to the same user ID, so we attempt to insert both.

This way, for a ENS user whose name is jack.eth, the members map will have two entries mapping to the same user ID value.

Addresses https://linear.app/comm/issue/ENG-5075/ensure-web-and-native-detect-ens-names-and-bold-them-for-markdown

Depends on D10388

Test Plan

Please see the videos below where I test on web and native

web

native

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan created this revision.Dec 18 2023, 12:04 PM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptDec 18 2023, 12:04 PM
rohan edited the test plan for this revision. (Show Details)Dec 18 2023, 12:10 PM
Harbormaster completed remote builds in B25192: Diff 34809.Dec 18 2023, 12:32 PM
rohan requested review of this revision.Dec 18 2023, 12:32 PM
ashoat added inline comments.Dec 18 2023, 12:36 PM
lib/shared/markdown.js
205–224 ↗(On Diff #34809)

This hook appears to return a new collection every time

web/markdown/rules.react.js
189 ↗(On Diff #34809)

This parameter doesn't appear to be used anymore. Should it be removed? And is there anywhere else it should also be removed?

rohan updated this revision to Diff 34846.Dec 19 2023, 7:11 AM
  1. Memoize hook return collection
  2. Remove members param from both web and native in rules.react.js
Harbormaster completed remote builds in B25226: Diff 34846.Dec 19 2023, 8:31 AM
rohan added a child revision: D10407: [keyserver] Update sendPushNotif to detect mention for a ENS name.Dec 19 2023, 9:18 AM
inka requested changes to this revision.Dec 21 2023, 2:32 AM

getDefaultTextMessageRules is exactly the same on native and web, can we move it to lib?

lib/shared/markdown.js
211–213 ↗(On Diff #34846)

Do we have a guarantee that useENSNames returns names in the same order?
I'd rather we turned resolvedMembers into a map keyed by id

218–224 ↗(On Diff #34846)

Why don't we have to check role in the second if?

If we should check role in the second if, can we just filter out the members with falsy role before call to useENSNames, to reduce the computation?

This revision now requires changes to proceed.Dec 21 2023, 2:32 AM
ashoat added a comment.Dec 21 2023, 6:29 AM
In D10389#301405, @inka wrote:

getDefaultTextMessageRules is exactly the same on native and web, can we move it to lib?

I think it references textMessageRules, which references fullMarkdownRules, which are different. Those could potentially be factored out I suppose, but not sure it's worth it

rohan updated this revision to Diff 34915.Dec 21 2023, 8:44 AM
rohan marked 2 inline comments as done.

Filter members without a role first, and construct a resolvedMembersMap and key by member.id

rohan added a comment.Dec 21 2023, 8:45 AM
In D10389#301405, @inka wrote:

getDefaultTextMessageRules is exactly the same on native and web, can we move it to lib?

I think it's the same as @ashoat said - getDefaultTextMessageRules looks at textMessageRules, which is different on web and native. For example, native returns some react-native specific components like Text. I think it's better to keep them separate for this reason

Harbormaster completed remote builds in B25278: Diff 34915.Dec 21 2023, 9:03 AM
inka accepted this revision.Dec 21 2023, 9:29 AM

I think it's the same as @ashoat said - getDefaultTextMessageRules looks at textMessageRules, which is different on web and native. For example, native returns some react-native specific components like Text. I think it's better to keep them separate for this reason

I see, thank you explaining. I think we can leave them, I don't feel strongly

This revision is now accepted and ready to land.Dec 21 2023, 9:29 AM
rohan updated this revision to Diff 34920.Dec 21 2023, 11:05 AM

Move constant to account-utils.js with a comment

rohan updated this revision to Diff 34921.Dec 21 2023, 11:07 AM

Revert the most recent update (updated the wrong diff)

Harbormaster completed remote builds in B25283: Diff 34920.Dec 21 2023, 11:23 AM
Harbormaster completed remote builds in B25284: Diff 34921.Dec 21 2023, 11:28 AM
Closed by commit rCOMM87417b194198: [lib/web/native] Bold ENS names in user mentions (authored by rohan). · Explain WhyDec 22 2023, 10:28 AM
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMM87417b194198: [lib/web/native] Bold ENS names in user mentions.

Revision Contents
Changeset List

PathSize
lib/
shared/
43 lines
native/
markdown/
19 lines
web/
markdown/
19 lines

Diff 34921

View Options

lib/shared/markdown.js

Loading...
View Options

native/markdown/rules.react.js

Loading...
View Options

web/markdown/rules.react.js

Loading...