Details

Reviewers

atul
tomek
rohan

Commits

rCOMMadeee6010336: [lib] introduce stringForReactionList in reaction-utils

Summary

introduce stringForReactionList in reaction-utils. This is a refactor where D5813 and D5814 were using the same logic in web and native, so we should put that logic into a helper function in lib. We then can call the function from web and native and get the same results

Depends on D5812
Linear Task: ENG-2247

Test Plan

Please to look at the screenshots to see that the reactions are still being rendered correctly

Native:

One reaction:

Screenshot 2022-12-15 at 6.38.32 PM.png (1×1 px, 796 KB)

Multiple reactions:

Screenshot 2022-12-15 at 6.38.25 PM.png (1×1 px, 797 KB)

Web:

One reaction:

Screenshot 2022-12-15 at 6.39.02 PM.png (2×3 px, 1 MB)

Multiple reactions:

Screenshot 2022-12-15 at 6.39.09 PM.png (2×3 px, 1 MB)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Dec 15 2022, 3:24 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 15 2022, 3:24 PM

Harbormaster completed remote builds in B14191: Diff 19420.Dec 15 2022, 3:37 PM

ginsu requested review of this revision.Dec 15 2022, 3:37 PM

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

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

ginsu added reviewers: atul, tomek, rohan.

ginsu added a parent revision: D5812: [native] introduced reactions to chat item height measurer.

ginsu attached a referenced file: F286865: Screenshot 2022-12-15 at 6.39.09 PM.png. (Show Details)

ginsu attached a referenced file: F286864: Screenshot 2022-12-15 at 6.39.02 PM.png. (Show Details)

ginsu attached a referenced file: F286863: Screenshot 2022-12-15 at 6.38.25 PM.png. (Show Details)

ginsu attached a referenced file: F286862: Screenshot 2022-12-15 at 6.38.32 PM.png. (Show Details)

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

rohan accepted this revision.Dec 16 2022, 8:14 AM

rohan added inline comments.

lib/shared/reaction-utils.js
16 ↗	(On Diff #19420)	Is `index > 0` going to only occur when there are multiple reactions? So if we have something like {"👍": `reactionInfo`, "👎": `reactionInfo`}, on the second iteration with the key being 👎, we'll append a space when displaying? I'm reviewing this a little out of context so feel free to correct me if I'm wrong!

ginsu added inline comments.Dec 16 2022, 8:47 AM

lib/shared/reaction-utils.js
16 ↗	(On Diff #19420)	Yup that is correct! Right now this if statement will always return false since we only have one reaction (👍), but when we start introducing more reactions we will want to place a space in between each reaction

This function seems like it'd be a great candidate for some unit testing + it would make it easier to review since there'd be example usages to step through

I think it'd be cleaner to construct an array with all the elements and then use blah.join(' ') instead of having to add spaces manually

This revision now requires changes to proceed.Dec 18 2022, 9:38 PM

address atul's feedback

In D5878#177124, @atul wrote:

This function seems like it'd be a great candidate for some unit testing + it would make it easier to review since there'd be example usages to step through

I can definitely do this, I will create a follow up diff to introduce the tests

Harbormaster completed remote builds in B14322: Diff 19565.Dec 19 2022, 7:32 AM

Much cleaner, thanks!

In D5878#177422, @ginsu wrote:

In D5878#177124, @atul wrote:

This function seems like it'd be a great candidate for some unit testing + it would make it easier to review since there'd be example usages to step through

I can definitely do this, I will create a follow up diff to introduce the tests

Makes sense, in the future it'd actually be helpful to include in the diff that introduces the function to make things easier to review.

lib/shared/reaction-utils.js
12	I think `reaction` instead of `key` would be more descriptive: for (const reaction of reactions.keys()) { Not a blocker though, whatever you prefer.

This revision is now accepted and ready to land.Dec 19 2022, 8:58 AM

changed variable name from key to reaction

Makes sense, in the future it'd actually be helpful to include in the diff that introduces the function to make things easier to review.

Gotcha in the future when I need to write more tests, I will keep this in mind

Harbormaster failed remote builds in B14433: Diff 19686!Dec 19 2022, 11:28 AM

Harbormaster completed remote builds in B14433: Diff 19686.Dec 19 2022, 11:43 AM

ginsu mentioned this in D5953: [lib] introduce unit tests for stringForReactionList reaction util function.Dec 20 2022, 9:16 AM

In D5878#177124, @atul wrote:

This function seems like it'd be a great candidate for some unit testing + it would make it easier to review since there'd be example usages to step through

D5953