Page MenuHomePhabricator

[web] Redesign `InlineSidebar` with reactions support
ClosedPublic

Authored by jacek on Aug 23 2022, 5:24 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 9:59 AM
Unknown Object (File)
Mon, Nov 25, 9:40 AM
Unknown Object (File)
Mon, Nov 25, 9:31 AM
Unknown Object (File)
Mon, Nov 25, 7:12 AM
Unknown Object (File)
Mon, Nov 11, 1:52 AM
Unknown Object (File)
Fri, Nov 8, 2:16 PM
Unknown Object (File)
Tue, Nov 5, 7:08 PM
Unknown Object (File)
Mon, Nov 4, 7:38 AM
Subscribers

Details

Summary

Introduce redesigned InlineSidebar component.
Currently it receives reactios as an array of strings to simplify testing with emojis, but it the future it will problably need to receive some complex rection data type.

image.png (631×483 px, 227 KB)

Test Plan

Tested text, media and robotext messages in all cases with/without sidebar or/and reactions.

Diff Detail

Repository
rCOMM Comm
Branch
jacek/inline-sidebar-web
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

jacek held this revision as a draft.
tomek requested changes to this revision.Aug 26 2022, 10:04 AM
tomek added inline comments.
web/chat/inline-sidebar.react.js
19 ↗(On Diff #15869)

Using [] as a default value breaks the memoization

25–27 ↗(On Diff #15869)

We're using center as a container type, so maybe use left and right to make it more abstract? CSS doesn't care too much about viewers

47–50 ↗(On Diff #15869)

Can we use it directly instead of memoizing it? Is the memoization of this component really beneficial?

62 ↗(On Diff #15869)

We don't need to include the whole threadInfo in dependencies. Just !threadInfo is what we care about.

68 ↗(On Diff #15869)

What value is returned by useOnClickThread when threadInfo is falsy? Do we need a conditional here?

77 ↗(On Diff #15869)

Maybe use a default import instead?

This revision now requires changes to proceed.Aug 26 2022, 10:04 AM

address feedback

web/chat/inline-sidebar.react.js
68 ↗(On Diff #15869)

useOnClickThread has invariant, so it cannot be called with falsy value:

invariant(
  thread?.id,
  'useOnClickThread should be called with threadID set',
);
This revision is now accepted and ready to land.Sep 2 2022, 3:43 AM