Page MenuHomePhabricator

[native] Display the number of pinned messages in a banner across chat
ClosedPublic

Authored by rohan on Apr 25 2023, 10:28 AM.
Tags
None
Referenced Files
F3359421: D7614.diff
Sun, Nov 24, 9:00 AM
F3358593: D7614.id25684.diff
Sun, Nov 24, 5:36 AM
F3358592: D7614.id.diff
Sun, Nov 24, 5:36 AM
F3358591: D7614.diff
Sun, Nov 24, 5:36 AM
Unknown Object (File)
Sun, Nov 10, 6:40 PM
Unknown Object (File)
Sun, Nov 10, 6:37 PM
Unknown Object (File)
Sun, Nov 10, 5:59 PM
Unknown Object (File)
Sun, Nov 10, 4:05 PM
Subscribers

Details

Summary

We want to display the number of pinned messages in a banner on top of chat the displays the number of pinned messages (if > 0 and if threadInfo contains a pinnedCount field since we gate it by hasMinCodeVersion on native).

The code is largely similar to on web: https://phab.comm.dev/D7310

Depends on D7608

Test Plan

Check the banner to confirm the banner does not show if there are no pinned messages, and confirm that the banner appears for both 1 and more than 1 pinned messages.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul requested changes to this revision.May 3 2023, 8:08 AM

Can we get rid of the React.useMemo(...) here?

native/chat/message-list-container.react.js
365–372 ↗(On Diff #25684)

You don't need to use React.useMemo(...) here.

The useMemo() hook is helpful for maintaining referential equality so that objects will be considered "shallowly equal" (== in JS) and we can avoid re-renders. This is helpful for objects (including Map(), Set(), etc), arrays (which are objects), and functions (which are objects).

On the other hand, strings in JS are considered shallowly equal if they have the same contents, so we don't have to worry about re-renders if the "content" stays the same.

See below:

875403.png (620×810 px, 104 KB)


(In like C++, which you've been working w/ recently, std::strings can be allocated on the heap (unlike integers, booleans, etc) which may have been part of your reasoning that shallow equality would check reference instead of contents?)

This revision now requires changes to proceed.May 3 2023, 8:08 AM
This revision is now accepted and ready to land.May 8 2023, 7:19 AM