[native] persist recently used emojis
ClosedPublic
Actions

Authored by ginsu on Jun 21 2023, 2:47 PM.

Details

Reviewers

atul
kamil

Commits

rCOMMbb40eafe9fd8: [native] persist recently used emojis

Summary

This diff persists the recently selected emojis from the emoji keyboard into async storage. To create this diff I read through the docs and found these examples right here:

I believe this should be an acceptable use case of using AsyncStorage (especially as a first step), as we are only storing a small amount of data, this seems like the recommended way to persist the recently used emojis from the documentation, and we are using a completely different emoji keyboard library for web. If we are interested in persisting the recently used emojis across web/native or different devices, then I can create a follow up task for this; however, I personally feel that this wouldn't really add much value to the overall user experience.

Linear Task: https://linear.app/comm/issue/ENG-3792/mobile-emoji-keyboard-recently-used-category

Test Plan

Please watch the demo video:

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Jun 21 2023, 2:47 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJun 21 2023, 2:47 PM

ginsu edited the summary of this revision. (Show Details)Jun 21 2023, 3:02 PM

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

ginsu added reviewers: atul, kamil.

ginsu attached a referenced file: F600827: Screen Recording 2023-06-21 at 3.00.30 PM.mov. (Show Details)

Harbormaster completed remote builds in B20409: Diff 27981.Jun 21 2023, 3:05 PM

ginsu requested review of this revision.Jun 21 2023, 3:06 PM

ginsu added inline comments.Jun 21 2023, 3:12 PM

native/components/emoji-keyboard.react.js
41–44 ↗	(On Diff #27981)	This callback was inspired from this example in the `rn-emoji-keyboard` codebase
46–54 ↗	(On Diff #27981)	This callback was inspired from this example in the `rn-emoji-keyboard` codebase

ginsu edited the summary of this revision. (Show Details)Jun 21 2023, 3:15 PM

ginsu added a child revision: D8284: [native] introudce selectMultipleEmojis and alreadySelectedEmojis to EmojiKeyboard component.Jun 21 2023, 3:58 PM

kamil accepted this revision.Jun 22 2023, 3:53 AM

kamil added inline comments.

native/components/emoji-keyboard.react.js
41–44 ↗	(On Diff #27981)	can this be defined outside component to avoid memoization? (has no dependencies) same thing for `onStateChangeCallback`

This revision is now accepted and ready to land.Jun 22 2023, 3:53 AM

ginsu added inline comments.Jun 22 2023, 4:32 PM

native/components/emoji-keyboard.react.js
41–44 ↗	(On Diff #27981)	Please correct me if I am wrong, but based on my understanding we only define constants at the top level scope (so whenever we use `React.useMemo`). I found some examples in the codebase where we have a `useCallback` w/o any dependencies in the dependencies list so I thought it was okay to do this https://github.com/CommE2E/comm/blob/master/native/account/registration/connect-ethereum.react.js#L110-L118 https://github.com/CommE2E/comm/blob/master/native/apps/apps-directory.react.js#L45 https://github.com/CommE2E/comm/blob/master/native/chat/message-list-thread-search.react.js#L85-L88 I can see though that since we aren't calling/using anything directly from the component in this callback it would make sense to define this outside the component (cc @ashoat @atul @tomek)

kamil added inline comments.Jun 23 2023, 2:06 AM

native/components/emoji-keyboard.react.js
41–44 ↗	(On Diff #27981)	Please correct me if I am wrong, but based on my understanding we only define constants at the top level scope (so whenever we use `React.useMemo`). I am not sure how callback will differ from constant. it's always better to avoid creating a function each time component is rendered (for the first time in this case) and memoizing it which is computationally expensive. I found some examples in the codebase where we have a `useCallback` w/o any dependencies in the dependencies list so I thought it was okay to do this https://github.com/CommE2E/comm/blob/master/native/account/registration/connect-ethereum.react.js#L110-L118 https://github.com/CommE2E/comm/blob/master/native/apps/apps-directory.react.js#L45 https://github.com/CommE2E/comm/blob/master/native/chat/message-list-thread-search.react.js#L85-L88 Looks like examples from those links either have to be defined inside the component (no dependencies but using `setState()`) or are some kind of one-off, here is the example of a callback defined outside: https://github.com/CommE2E/comm/blob/a5f9b1e53b7fdb6ac335dcc546b93ecef4b1dd6a/native/chat/text-message-tooltip-modal.react.js#L32 I can see though that since we aren't calling/using anything directly from the component in this callback it would make sense to define this outside the component In fact, there is a better solution (suggested by @tomek) - see my inline comment below.
56–59 ↗	(On Diff #27981)	There is not a lot of profit from memoizing callbacks when here there is a new object created each time, and also without any dependencies. Maybe creating this object (`useRecentPicksPersistence` argument) outside, with functions, with a descriptive name will be the best?

ginsu added inline comments.Jun 23 2023, 11:35 AM

native/components/emoji-keyboard.react.js
56–59 ↗	(On Diff #27981)	Okay this is a great idea, and thank you for sharing the example with the callback defined outside, that definitely makes a lot of sense now. Will update the diff shortly

address feedback

Harbormaster completed remote builds in B20474: Diff 28056.Jun 23 2023, 12:00 PM

rebase before landing

This revision was landed with ongoing or failed builds.Jun 23 2023, 1:09 PM

Closed by commit rCOMMbb40eafe9fd8: [native] persist recently used emojis (authored by ginsu). · Explain Why

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMMbb40eafe9fd8: [native] persist recently used emojis.

Harbormaster completed remote builds in B20481: Diff 28064.Jun 23 2023, 1:23 PM

ashoat added inline comments.Jun 28 2023, 3:22 PM

native/components/emoji-keyboard.react.js
28–33 ↗	(On Diff #28067)	`async` / `await` probably isn't necessary here, you can just forward the promise. It's important to remember that `async` / `await` syntax is mostly just syntactic sugar around promises