Page MenuHomePhabricator

[lib] Include users mentioned in source message in sidebar
ClosedPublic

Authored by ashoat on Feb 17 2023, 8:54 AM.
Tags
None
Referenced Files
F3252466: D6763.diff
Fri, Nov 15, 6:20 PM
Unknown Object (File)
Fri, Nov 1, 10:21 AM
Unknown Object (File)
Wed, Oct 30, 12:51 AM
Unknown Object (File)
Wed, Oct 30, 12:51 AM
Unknown Object (File)
Wed, Oct 30, 12:51 AM
Unknown Object (File)
Wed, Oct 30, 12:51 AM
Unknown Object (File)
Wed, Oct 30, 12:50 AM
Unknown Object (File)
Sep 30 2024, 8:17 AM
Subscribers

Details

Summary

This addresses ENG-3032.

Depends on D6762

Test Plan
  1. Log the list of members at the end of `createPendingSidebar
  2. Tested in combination with the following diffs, where I show screenshots of a pending sidebar with the @-mentions bolded

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/shared/thread-utils.js
503–519 ↗(On Diff #22682)

Note that this function (baseCreatePendingSidebar) gets called from two places: createPendingSidebar and createUnresolvedPendingSidebar.

In the former case we are able to do ENS resolution really easily, so I initially implemented this code so that ENS names were resolved in the former case but not the latter.

But then I realized that bolding text can change its height, and we need the height calculation to be consistent between these two functions in order for the sidebar animation to look correct.

To do this right we'd probably want to trigger remeasurement of a message once ENS names come in, but doing this in a performant way will probably require adding a new provider component upstream of MessageListContainer that can handle the ENS querying and avoid repeated remeasures of the same (unchanged) component. We'll also need to think about the measureKey, which needs to include a list of mentionableNames somehow in order for the height-measuring code to realize the height may be different.

Since the scope there is pretty high, and since we don't support @-mentioning ENS names yet anyways, I've decided to defer that work to ENG-1969.

atul added inline comments.
lib/shared/thread-utils.js
503–519 ↗(On Diff #22682)

Thanks for the thorough explanation + linking the Linear issue

This revision is now accepted and ready to land.Feb 20 2023, 2:24 PM