Details

Reviewers

inka
ashoat

Commits

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

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

ginsu created this revision.May 6 2024, 12:16 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptMay 6 2024, 12:16 AM

ginsu edited the summary of this revision. (Show Details)May 6 2024, 12:22 AM

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

ginsu added reviewers: inka, ashoat.

ginsu added a parent revision: D11893: [native] improve error handling for TagUnfollowedFarcasterChannel screen.

ginsu attached a referenced file: F1707026: Screen Recording 2024-05-06 at 3.21.05 AM.mov. (Show Details)

ginsu added inline comments.May 6 2024, 12:25 AM

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

Harbormaster completed remote builds in B28684: Diff 39828.May 6 2024, 12:36 AM

ginsu requested review of this revision.May 6 2024, 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.

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.May 6 2024, 12:13 PM

address comments

ginsu retitled this revision from [native] introduce tag channel by name link to [native] introduce tag channel by name option in action sheet.May 13 2024, 10:55 PM

ginsu edited the summary of this revision. (Show Details)

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

ginsu attached a referenced file: F1750982: Screen Recording 2024-05-14 at 1.46.13 AM.mov. (Show Details)

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

ginsu edited the summary of this revision. (Show Details)May 13 2024, 10:58 PM

ginsu edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B28885: Diff 40144.May 13 2024, 11:04 PM

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.May 14 2024, 9:35 AM

ashoat added inline comments.May 14 2024, 9:36 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?

ginsu added a child revision: D11903: [keyserver] introduce a table for communities.May 15 2024, 2:35 PM

ginsu added inline comments.May 15 2024, 2:40 PM

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

address comments

ginsu edited the test plan for this revision. (Show Details)May 15 2024, 2:45 PM

ginsu attached a referenced file: F1771282: Screenshot 2024-05-15 at 5.28.31 PM.png. (Show Details)

Harbormaster completed remote builds in B28963: Diff 40260.May 15 2024, 2:57 PM

ashoat accepted this revision.May 15 2024, 5:14 PM

ashoat added inline comments.

native/community-settings/tag-farcaster-channel/tag-farcaster-channel.react.js
122–139	This would've been easier to review if you separate the move out into its own diff. See here
132–135	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	It's confusing that you take this out and then bring it back in a later diff