Details

Reviewers

tomek
ashoat
atul

Commits

rCOMM2e65a4c16214: [web] introduce useOnClickReact hook in reaction message utils

Summary

intorduce useOnClickReact hook. useOnClickReact is the web implmentation of the similar function onPressReact introduced in D5780. This function handles the calling of the create reaction message endpoint and then dispatching the results of that to the reducer

Linear Task: ENG-2473

Depends on D6054

Test Plan

This task will be futher tested in subsequent diffs. I was also able to like/unlike messages in web in my local stack using this hook and some subsequent diffs

Additionally, here is what the Alert looks like if a message fails to go through

Screenshot 2022-12-28 at 12.11.26 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 27 2022, 7:21 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 27 2022, 7:21 AM

ginsu edited the summary of this revision. (Show Details)Dec 27 2022, 7:25 AM

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

ginsu added reviewers: atul, tomek.

ginsu added a parent revision: D6054: [lib/native] move useCanCreateReactionFromMessage hook from native to lib.

ginsu attached a referenced file: F306362: Screen Recording 2022-12-27 at 10.24.44 AM.mov. (Show Details)

Harbormaster completed remote builds in B14802: Diff 20188.Dec 27 2022, 7:35 AM

ginsu requested review of this revision.Dec 27 2022, 7:35 AM

ginsu added a child revision: D6056: [web] intoduce MessageLikeTooltipButton component.Dec 27 2022, 10:29 AM

ashoat added inline comments.Dec 27 2022, 1:20 PM

web/chat/reaction-message-utils.js
42 ↗	(On Diff #20188)	On web we usually use `pushModal` instead and show something like eg. `ConcurrentModificationModal`. We don't currently have any usages of `alert` in the web codebase Can you either: Attach some videos of what this looks like in Chrome, Firefox, Safari Or update it to match existing `web` conventions Not trying to slow you down but I think we should think this through

ginsu edited the test plan for this revision. (Show Details)Dec 27 2022, 3:35 PM

ginsu attached a referenced file: F306701: Screen Recording 2022-12-27 at 6.33.06 PM.mov. (Show Details)

ginsu attached a referenced file: F306700: Screen Recording 2022-12-27 at 6.32.39 PM.mov. (Show Details)

ginsu attached a referenced file: F306699: Screen Recording 2022-12-27 at 6.32.26 PM.mov. (Show Details)

ginsu added inline comments.Dec 27 2022, 3:37 PM

web/chat/reaction-message-utils.js
42 ↗	(On Diff #20188)	Not trying to slow you down but I think we should think this through No definitely this is a good shout. Just updated the test plan to show what alerts look like in Chrome, Firefox and Safari. If we don't think alerts are a good user experience, will be happy to update the code based off the existing `web` conventions

Safari looks good but the others look really bad... I think we should probably stick with the current pattern

In D6055#181773, @ashoat wrote:

Safari looks good but the others look really bad... I think we should probably stick with the current pattern

Gotcha I can definitely do that

Making changes to improve the error alert

updated alert to use in house Alert component

ginsu edited the test plan for this revision. (Show Details)Dec 28 2022, 9:15 AM

ginsu attached a referenced file: F308027: Screenshot 2022-12-28 at 12.11.26 PM.png. (Show Details)

Harbormaster completed remote builds in B14843: Diff 20243.Dec 28 2022, 9:29 AM

update to fix bug in D6057

Harbormaster completed remote builds in B14852: Diff 20257.Dec 28 2022, 12:30 PM

ashoat requested changes to this revision.Dec 28 2022, 5:18 PM

ashoat added inline comments.

web/chat/reaction-message-utils.js
51	We should still `throw e`... otherwise we implicitly return `undefined` (wish we had an ESLint rule that forced these to be explicit), and as a result will dispatch a `_SUCCESS` action with an `undefined` `action.payload`, instead of a `_FAILED` action with the exception info