Details

Reviewers

atul
rohan

Commits

rCOMMce3b411fe5ed: [native] deduped open thread and create thread option

Summary

deduped open_thread and create_thread to just be thread. This is because both create_thread and open_thread have the same function. This diff was inspired by this comment on D5381

Depends on D5381
Linear Task: ENG-2037

Test Plan

Please watch the video demo to see that there are no visaul or performance regressions:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Oct 20 2022, 2:30 PM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptOct 20 2022, 2:30 PM

ginsu edited the summary of this revision. (Show Details)Oct 20 2022, 2:33 PM

ginsu added reviewers: atul, rohan.

ginsu added a parent revision: D5381: [native] switched fixed tooltip component from a vertical layout to horizontal.

ginsu added inline comments.Oct 20 2022, 2:37 PM

native/chat/text-message.react.js
124–125 ↗	(On Diff #17793)	removed this if condition because `this.props.canCreateSidebarFromMessage` will always be true for all messages, while `this.props.item.threadCreatedFromMessage` will not always be true
native/navigation/tooltip.react.js
132–133 ↗	(On Diff #17793)	we need to keep these conditions because `MultimediaMessageTooltipModal` and `RobotextMessageTooltipModal` still use `create_sidebar` and `open_sidebar`

Harbormaster completed remote builds in B12952: Diff 17793.Oct 20 2022, 2:40 PM

ginsu requested review of this revision.Oct 20 2022, 2:40 PM

ashoat added inline comments.Oct 20 2022, 2:47 PM

native/navigation/tooltip.react.js
132–133 ↗	(On Diff #17793)	Do they need to? Why don't we dedup for them too?

ginsu planned changes to this revision.Oct 20 2022, 2:49 PM

ginsu added inline comments.

native/navigation/tooltip.react.js
132–133 ↗	(On Diff #17793)	okay i can do that!

addressed ashoat's comments

ginsu retitled this revision from [native] deduped open thread and create thread option for normal text message to [native] deduped open thread and create thread option.Oct 20 2022, 2:59 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu attached a referenced file: F205209: Screen Recording 2022-10-20 at 5.56.09 PM.mov. (Show Details)

ginsu added inline comments.

native/chat/multimedia-message.react.js
85–87 ↗	(On Diff #17794)	removed this if condition because `this.props.canCreateSidebarFromMessage` will always be true for all messages, while `this.props.item.threadCreatedFromMessage` will not always be true

Harbormaster completed remote builds in B12953: Diff 17794.Oct 20 2022, 3:09 PM

ginsu added a child revision: D5452: [native] removed tooltip location logic.Oct 20 2022, 3:16 PM

Don't think the following assumption is correct:

removed this if condition because this.props.canCreateSidebarFromMessage will always be true for all messages, while this.props.item.threadCreatedFromMessage will not always be true

native/chat/multimedia-message.react.js
85–87 ↗	(On Diff #17794)	`this.props.canCreateSidebarFromMessage` will always be true for all messages Ah this isn't always true. For example if you go to one of the sidebars in `Daily Updates` you'll see that you can't create a sidebar from those messages. We probably want something like: if (this.props.item.threadCreatedFromMessage \|\| this.props.canCreateSidebarFromMessage) { result.push('sidebar'); } ? Let me know if I'm missing something here!\