Details

Reviewers

inka
kamil
ashoat

Commits

rCOMM406620819a1b: [web] introduce CreateFarcasterChannelTagModal

Summary

This diff introduces a new modal component called CreateFarcasterChannelTagModal. CreateFarcasterChannelTagModal handles the user flow of creating a farcaster channel tag based on the farcaster channels you are following. Once the tag is successfully created then the modal will close and return the user to the TagFarcasterChannelModal modal

Depends on D12534

Test Plan

Please see the demo video below

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Jun 24 2024, 10:36 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJun 24 2024, 10:36 PM

Harbormaster completed remote builds in B29913: Diff 41673.Jun 24 2024, 10:52 PM

ginsu requested review of this revision.Jun 24 2024, 10:52 PM

This diff contains copy that needs to be reviewed

web/tag-farcaster-channel/create-farcaster-channel-tag-modal.react.js
106 ↗	(On Diff #41673)	This copy needs to be reviewed
115 ↗	(On Diff #41673)	This copy needs to be reviewed
web/tag-farcaster-channel/tag-farcaster-channel-modal.react.js
67 ↗	(On Diff #41673)	This copy needs to be reviewed

ginsu mentioned this in D12567: [web] introduce tagFarcasterChannelByName to CreateFarcasterChannelTagModal.Jun 24 2024, 11:17 PM

ginsu added a child revision: D12567: [web] introduce tagFarcasterChannelByName to CreateFarcasterChannelTagModal.

inka accepted this revision.Jun 27 2024, 6:41 AM

Had three concerns, but don't think any of them make sense to block this diff given @ginsu is leaving imminently:

I don't loved the nested modal UX. It feels like this could've been done a single modal, without requiring the nesting of modals
Some of the strings are copy-pasted, but not all of them. Given there are some differences in the UI, and given the strings are short, I figure it's okay to copy-paste
The invariant might potentially fire in a scenario where the user's Farcaster account is unlinked while they're on this screen. But that seems unlikely, and I think it would be resolved with a simple refresh

This revision is now accepted and ready to land.Jun 27 2024, 12:25 PM

ashoat added inline comments.Jun 27 2024, 1:12 PM

web/tag-farcaster-channel/create-farcaster-channel-tag-modal.css
13 ↗	(On Diff #41673)	Why do we need to hardcode height here?

ginsu added inline comments.Jun 27 2024, 1:57 PM

web/tag-farcaster-channel/create-farcaster-channel-tag-modal.css
13 ↗	(On Diff #41673)	This was used to make sure that the height of the modal doesn't change when an error message is shown; however, I saw that in D12567, you mentioned we should use `visibility: hidden` over hardcoding the height here so will update this diff to fix this before landing