Page MenuHomePhabricator

[native] introduce tag channel by name option in action sheet
AcceptedPublic

Authored by ginsu on Mon, May 6, 12:16 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 17, 7:58 AM
F1771282: Screenshot 2024-05-15 at 5.28.31 PM.png
Wed, May 15, 2:45 PM
F1750982: Screen Recording 2024-05-14 at 1.46.13 AM.mov
Mon, May 13, 10:55 PM
Unknown Object (File)
Sun, May 12, 1:38 AM
Unknown Object (File)
Sat, May 11, 4:46 PM
Unknown Object (File)
Thu, May 9, 9:19 AM
Unknown Object (File)
Mon, May 6, 1:36 AM
Unknown Object (File)
Mon, May 6, 1:36 AM
Subscribers

Details

Reviewers
inka
ashoat
Summary

This diff introduces removes all the unnecessary UI elements and introduces a new "Other" option in the action sheet. When the user presses on the"Other" option they will be navigated to the TagFarcasterChannelByName screen

If the user presses on a channel name then a farcaster channel tag will be created with that channel

The UI will continue to be updated + improved in a subsequent diff

Depends on D11893

Test Plan

Confirmed that a farcaster channel tag was created if a channel name was selected:

EDIT: here is a screenshot with the updated /CHANNEL_ID format:

Screenshot 2024-05-15 at 5.28.31 PM.png (1×1 px, 775 KB)

Please see the demo video below

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

native/community-settings/tag-farcaster-channel/tag-farcaster-channel.react.js
211 ↗(On Diff #39828)

This copy needs to be reviewed.

Also please note that the space is intentional

214 ↗(On Diff #39828)

This copy needs to be reviewed

ginsu requested review of this revision.Mon, May 6, 12:36 AM

What happens if a name is chosen on the TagFarcasterChannelByName screen?

What happens if a name is chosen on the TagFarcasterChannelByName screen?

When the user presses the "Tag channel" button in TagFarcasterChannelByName we will dispatch the create or update farcaster channel tag action and when then when we get a successful response we will go back to this screen, where the user will be able to see the existing newly created tag (showing the existing tag will be introduced in a subsequent diff and is tracked in this linear task: https://linear.app/comm/issue/ENG-7946/display-the-currently-tagged-farcaster-channel-in-the-tag-farcaster ).

The implementation for this callback was introduced in D11866.

ashoat requested changes to this revision.Mon, May 6, 12:13 PM

I think it's not obvious that the way to find a channel is to press the pencil button. This is made extra confusing by showing the "Can't find a channel?" prompt. Where exactly should we be expected to find a channel?

This UI is confusing and should be revised

This revision now requires changes to proceed.Mon, May 6, 12:13 PM
ginsu retitled this revision from [native] introduce tag channel by name link to [native] introduce tag channel by name option in action sheet.Mon, May 13, 10:55 PM
ginsu edited the summary of this revision. (Show Details)
ginsu edited the test plan for this revision. (Show Details)
ginsu edited the test plan for this revision. (Show Details)
ginsu edited the summary of this revision. (Show Details)
ashoat requested changes to this revision.Tue, May 14, 9:35 AM

Let's use eg. /comm instead of Comm. Besides matching Farcaster conventions better, this also lets the user understand more clearly that "Other" isn't a channel name

native/community-settings/tag-farcaster-channel/tag-farcaster-channel.react.js
132 ↗(On Diff #40144)

Let's use channel.id instead of channel.name, and prefix it with a slash

187 ↗(On Diff #40144)

Where's the loading state for this?

This revision now requires changes to proceed.Tue, May 14, 9:35 AM
native/community-settings/tag-farcaster-channel/tag-farcaster-channel.react.js
134 ↗(On Diff #40144)

Does @expo/react-native-action-sheet document a limit to the length of this array? Is it the same on iOS and Android?

137 ↗(On Diff #40144)

Why aren't we including Other on Android?

native/community-settings/tag-farcaster-channel/tag-farcaster-channel.react.js
134 ↗(On Diff #40144)

There should be no limit to the length of this array. The action sheet is scrollable to handle a large number of elements in this array.

Confirmed this on both ios and android using the following expo snack:
https://snack.expo.dev/KH6EXkolM8y7D9MqLUsPy

137 ↗(On Diff #40144)

This was a mistake, will push an update to fix this

187 ↗(On Diff #40144)

In D12024, I introduced a new TagChannelButton component that replaces this, and then in D12025 I implement the loading state for this button

ashoat added inline comments.
native/community-settings/tag-farcaster-channel/tag-farcaster-channel.react.js
122–139 ↗(On Diff #40260)

This would've been easier to review if you separate the move out into its own diff. See here

132–135 ↗(On Diff #40260)

Most Farcaster users follow a ton of channels – can you make sure that the list of alphabetized, and make sure Other appears on the first load? (If it starts with the options at the start then make Other first)

180–190 ↗(On Diff #40260)

It's confusing that you take this out and then bring it back in a later diff

This revision is now accepted and ready to land.Wed, May 15, 5:14 PM

address comments + rebase before landing