[web] introduce CreateFarcasterChannelTagModal
AcceptedPublic
Actions

Authored by ginsu on Mon, Jun 24, 10:36 PM.

Details

Reviewers

inka
kamil
ashoat

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 D12565

Test Plan

Please see the demo video below

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

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

Herald added subscribers: tomek, ashoat. · View Herald TranscriptMon, Jun 24, 10:36 PM

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

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

This diff contains copy that needs to be reviewed

web/tag-farcaster-channel/create-farcaster-channel-tag-modal.react.js
106	This copy needs to be reviewed
115	This copy needs to be reviewed
web/tag-farcaster-channel/tag-farcaster-channel-modal.react.js
67	This copy needs to be reviewed

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

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

inka accepted this revision.Thu, Jun 27, 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.Thu, Jun 27, 12:25 PM

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

web/tag-farcaster-channel/create-farcaster-channel-tag-modal.css
13	Why do we need to hardcode height here?

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

web/tag-farcaster-channel/create-farcaster-channel-tag-modal.css
13	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