[native] modified tooltip UI to show 'Unlike' when user already likes a message
ClosedPublic
Actions

Authored by ginsu on Dec 20 2022, 3:07 PM.

Details

Reviewers

atul
tomek

Commits

rCOMM3799d36a136f: [native] modified tooltip UI to show 'Unlike' when user already likes a message

Summary

modified the tooltip react entry to either show 👍 or Unlike depending on whether the user liked the message. Happy to make this look better, but I figured since this is a temporary solution that will be reverted once we go from message liking to message reactions, it wasn't worth spending a ton of time to make this perfect.

I also modified the destructiveButtonIndex to only have a value if the Report option is present. As I was making the changes above, I noticed that Unlike was turning red when a viewer was trying to unlike their own message.

Depends on D5813

Test Plan

Please watch the demo video to see the changes I made:

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Dec 20 2022, 3:07 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 20 2022, 3:07 PM

ginsu edited the summary of this revision. (Show Details)Dec 20 2022, 3:16 PM

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

ginsu added reviewers: atul, tomek.

ginsu added a parent revision: D5813: [native] introduce reactions to inline sidebar.

ginsu attached a referenced file: F297006: Screen Recording 2022-12-20 at 6.08.39 PM.mov. (Show Details)

ginsu attached a referenced file: F297015: Screen Recording 2022-12-20 at 6.15.54 PM.mov. (Show Details)

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 20 2022, 3:20 PM

Harbormaster failed remote builds in B14588: Diff 19894!

Harbormaster completed remote builds in B14588: Diff 19894.Dec 20 2022, 3:32 PM

ginsu requested review of this revision.Dec 20 2022, 3:32 PM

tomek requested changes to this revision.Dec 21 2022, 8:27 AM

tomek added inline comments.

native/navigation/tooltip.react.js
257–266 ↗	(On Diff #19894)	This code is really specific to one kind of entry. Can we move this logic somewhere else - preferably to the place where entries are defined, so that we don't have to modify them here?
660 ↗	(On Diff #19894)	Shouldn't we return `reportIndex`?

This revision now requires changes to proceed.Dec 21 2022, 8:27 AM

address tomek's feedback

ginsu added inline comments.Dec 21 2022, 11:50 AM

native/navigation/tooltip.react.js
257–266 ↗	(On Diff #19894)	Unfortunately, because we define the entries in an object outside of a react component (here's an example with the text message tooltip modal) I don't think it's possible to move the logic where the entries are defined. I do truly see this as a temporary solution and once we extend from message likes to message reactions in a few weeks, we will be able to remove this code. However, if my understanding of not being able to move the logic where the entires are defined is incorrect please let me know, and I will take a deeper dive into this.
660 ↗	(On Diff #19894)	I updated this code to use `reportIndex` in the ternary expression's return. This should make the solution more airtight and future-proof; however, because we are reversing the order of the entries on line 600 we need to use `options.length - reportIndex` instead of just `reportIndex`

Harbormaster completed remote builds in B14642: Diff 19973.Dec 21 2022, 11:53 AM

ginsu mentioned this in D5920: [lib] set min code version for message liking.Dec 21 2022, 2:46 PM

tomek requested changes to this revision.Dec 22 2022, 8:01 AM

tomek added inline comments.

native/navigation/tooltip.react.js
257–266 ↗	(On Diff #19894)	I don't think it makes too much sense to spend a lot of time investigating something which we consider a temporary solution.
660 ↗	(On Diff #19894)	This sounds really counter-intuitive to return inverted number here just because of how it is used. We should instead return the proper index from here and take care of inversing it while reversing the collection. But that should be done later, as a separate task and doesn't block this stack in any way. Shouldn't we also subtract 1 from the result? I guess it might be needed because when we consider an edge case of just a single button with report, then the length is 1, report index is 0 and we would return 1 from this function which is >= than the array's length. Or maybe I'm missing something and it works correctly - have you tested it after your change?

This revision now requires changes to proceed.Dec 22 2022, 8:01 AM

ginsu requested review of this revision.Dec 22 2022, 9:55 AM

ginsu added inline comments.

native/navigation/tooltip.react.js
660 ↗	(On Diff #19894)	But that should be done later, as a separate task and doesn't block this stack in any way. We actually have a task to simplify the `ActionSheet` and remove duplicate options, which will get rid of all this complicated/counter-intuitive reversing logic Shouldn't we also subtract 1 from the result? I guess it might be needed because when we consider an edge case of just a single button with report, then the length is 1, report index is 0 and we would return 1 from this function which is >= than the array's length. Or maybe I'm missing something and it works correctly - have you tested it after your change? We don't need to subtract 1 from the result because we will never hit this edge case, since the `ActionSheet` conditionally renders if there are more than 3 entries. When I made these changes, I tested, and everything still works correctly, and the demo in the test plan is still accurate.

(Removing myself as blocking, feel free to re-add if there's something specific)

Accepting, but please test that one scenario from my comment.

native/navigation/tooltip.react.js
660 ↗	(On Diff #19894)	We don't need to subtract 1 from the result because we will never hit this edge case, since the ActionSheet conditionally renders if there are more than 3 entries. When I made these changes, I tested, and everything still works correctly, and the demo in the test plan is still accurate. It was just an example showing a potential bug. We have to make sure that the code is correct and not seems to be correct just because actions are rendered in specific order. Could you check if this still works correctly when report action is on 0th index?

This revision is now accepted and ready to land.Dec 23 2022, 1:40 AM

ginsu added inline comments.Dec 27 2022, 4:33 PM

native/navigation/tooltip.react.js
660 ↗	(On Diff #19894)	Here's a demo video that covers your scenario. I put the report action as the 0th index of `entries`, and the `destructiveButtonIndex` still returned the correct place. Hoping that this should clear things up, but let me know if you have any more follow up questions