Page MenuHomePhabricator
Differential D4824

[native] Redesign `InlineSidebar` for reactions feature
ClosedPublic
Actions

Authored by • jacek on Aug 12 2022, 2:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 6, 3:36 AM
Unknown Object (File)
Nov 14 2024, 6:46 PM
Unknown Object (File)
Nov 10 2024, 12:22 AM
Unknown Object (File)
Nov 9 2024, 11:05 PM
Unknown Object (File)
Nov 9 2024, 9:17 PM
Unknown Object (File)
Nov 9 2024, 9:15 PM
Unknown Object (File)
Nov 9 2024, 9:07 PM
Unknown Object (File)
Nov 9 2024, 7:37 PM
View All Files
Subscribers
• adrian
ashoat

Details

Reviewers
tomek
atul
• abosh
ashoat
Commits
rCOMMa9a0c58936cc: [native] Redesign `InlineSidebar` for reactions feature
Summary

Introduce redesigned InlineSidebar component for native. It is now able to keep both sidebar and reactions.
Now it receives reactions as an array of strings - so it would be easy to test visually by providing an array of emojis. Probably, in the future we will keep reactions in some more complex data structure, so the types should be adjusted then.

Screenshot_Simulator_2022-08-12_115809.png (588×565 px, 33 KB)

Screenshot_Simulator_2022-08-12_115833.png (262×571 px, 26 KB)

Screenshot_Simulator_2022-08-12_120031.png (777×553 px, 481 KB)

Screenshot_Simulator_2022-08-12_120112.png (163×393 px, 12 KB)

GIF:
Screenshot_Simulator_2022-08-12_120400.gif (160×560 px, 620 KB)

Test Plan

Tested the component look on both platforms in both color themes. Also provided smoji and tested displaying reactions and/or sidebar.
The visual issues when opening message tooltip will be fixed in next diff.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• jacek created this revision.Aug 12 2022, 2:33 AM
• jacek held this revision as a draft.
Herald added subscribers: • adrian, ashoat. · View Herald TranscriptAug 12 2022, 2:33 AM
Harbormaster completed remote builds in B11338: Diff 15598.Aug 12 2022, 2:42 AM
• jacek updated this revision to Diff 15600.Aug 12 2022, 2:56 AM

position adjustments

Harbormaster completed remote builds in B11340: Diff 15600.Aug 12 2022, 3:03 AM
• jacek edited the summary of this revision. (Show Details)Aug 12 2022, 3:04 AM
• jacek edited the summary of this revision. (Show Details)
• jacek published this revision for review.Aug 12 2022, 3:13 AM
• jacek added a child revision: D4825: [native] Introduce `inlineSidebar` animation when opening message tooltip to avoid visual issues..
• jacek added parent revisions: D4823: [lib] Make `useInlineSidebarText` hook parameter optional, D4806: [native] Fix message tooltip positioning for robotext with sidebar.
• abosh accepted this revision.Aug 12 2022, 9:31 AM
• abosh added 2 blocking reviewer(s): atul, tomek.

Adding @atul and @tomek as blocking to get at least one of them to review this.

native/chat/composed-message.react.js
150–154 ↗(On Diff #15600)

Should this be memoized because its dependent on the value of inlineSidebarPositionStyle?

ashoat added a comment.Aug 12 2022, 10:18 AM

Would landing this diff cause fake placeholder reaction emojis to appear on prod?

ashoat requested changes to this revision.Aug 12 2022, 10:20 AM

Seeing @abosh's screenshots in D4825 make me fear the answer is "yes". Please do not land something like this on master...

This revision now requires changes to proceed.Aug 12 2022, 10:20 AM
• abosh added a comment.Aug 12 2022, 10:28 AM
In D4824#139480, @ashoat wrote:

Seeing @abosh's screenshots in D4825 make me fear the answer is "yes". Please do not land something like this on master...

There are no placeholder emojis, I supplied the array of emojis as props to InlineSidebar myself. I also left a similar comment on D4825.

ashoat accepted this revision.Aug 12 2022, 11:02 AM

My bad!

This revision now requires review to proceed.Aug 12 2022, 11:02 AM
atul requested changes to this revision.Aug 15 2022, 9:24 AM

Tried out the changes in the iOS Simulator and it looks really good!

Couple of questions inline, feel free to re-request review if I'm misunderstanding anything.

My understanding is that the values in native/chat/chat-constants.js (eg inlineSidebarMarginTop) were in there because they were used in other places (eg textMessageItemHeight).

Are values like inlineSidebarLeftTopOffset also expected to be used outside of the ComposedMessage component? If not, can we just set the values in the ComposedMessage styles object to avoid indirection?

As an aside, do any of the changes you've made affect functions like native/chat/multimedia-message-utils:multimediaMessageItemHeight(...) or native/chat/utils:textMessageItemHeight(...)?

native/chat/composed-message.react.js
16–25 ↗(On Diff #15600)

Can we encapsulate these constants in some sort of object to clean up exports/imports?

150–154 ↗(On Diff #15600)

ComposedMessage is currently a class component (though at a glance it probably wouldn't be too difficult to convert to a functional component)

native/chat/inline-sidebar.react.js
37 ↗(On Diff #15600)

Personally prefer idx (or index) over i, but really doesn't matter.. whatever you prefer.

56 ↗(On Diff #15600)

Could probably memoize this style outside of sidebarInfo?

This revision now requires changes to proceed.Aug 15 2022, 9:24 AM
• jacek added a comment.Aug 16 2022, 4:03 AM
In D4824#139635, @atul wrote:

Are values like inlineSidebarLeftTopOffset also expected to be used outside of the ComposedMessage component? If not, can we just set the values in the ComposedMessage styles object to avoid indirection?

Yes, these values will be used in TooltipInlineSidebar in next diff to correctly set its position.

As an aside, do any of the changes you've made affect functions like native/chat/multimedia-message-utils:multimediaMessageItemHeight(...) or native/chat/utils:textMessageItemHeight(...)?

These functions should still work correctly, and I didn't notice any problems with incorrect height measurements during tests, as I also updated inlineSidebarHeight.

• jacek updated this revision to Diff 15708.Aug 17 2022, 4:27 AM

Address Atul's suggestions

Harbormaster completed remote builds in B11419: Diff 15708.Aug 17 2022, 4:36 AM
atul accepted this revision.Aug 17 2022, 9:41 AM

Looks good!

tomek requested changes to this revision.Aug 19 2022, 7:50 AM
tomek added inline comments.
native/chat/inline-sidebar.react.js
39 ↗(On Diff #15708)

Not sure about using idx as a key here. If this component become stateful and we insert a new reaction in the middle / remove a reaction from a list, we might have a bug. What about using reaction as a key?

63 ↗(On Diff #15708)

We don't need to compute this memo when threadInfo is changed, so it would be more performant if we had a variable equal to !thread and used it as a dependency.

This revision now requires changes to proceed.Aug 19 2022, 7:50 AM
• jacek updated this revision to Diff 15909.Aug 24 2022, 2:23 AM

Address Tomek's comments

Harbormaster completed remote builds in B11571: Diff 15909.Aug 24 2022, 2:33 AM
tomek accepted this revision.Aug 24 2022, 2:46 AM
This revision is now accepted and ready to land.Aug 24 2022, 2:46 AM
Closed by commit rCOMMa9a0c58936cc: [native] Redesign `InlineSidebar` for reactions feature (authored by • jacek). · Explain WhyAug 25 2022, 1:39 AM
This revision was automatically updated to reflect the committed changes.
• jacek added a commit: rCOMMa9a0c58936cc: [native] Redesign `InlineSidebar` for reactions feature.

Revision Contents
Changeset List

Diff 15956

View Options

native/chat/chat-constants.js

Loading...
View Options

native/chat/composed-message.react.js

Loading...
View Options

native/chat/inline-sidebar.react.js

Loading...
View Options

native/chat/multimedia-message-utils.js

Loading...
View Options

native/chat/robotext-message.react.js

Loading...
View Options

native/chat/utils.js

Loading...
View Options

native/themes/colors.js

Loading...