Paths

Table of Contentst

Differential D12041

[native] display existing farcaster channel tag in TagFarcasterChannel screen
ClosedPublic
Actions

Authored by ginsu on May 15 2024, 1:04 AM.

Details

Reviewers

tomek
inka
ashoat

Commits

rCOMM78bd59beb331: [native] display existing farcaster channel tag in TagFarcasterChannel screen

Summary

When the user goes to the TagFarcasterChannel screen they should be able to see if the community already has a farcaster channel already associated with it. This diff uses the farcasterChannelID stored from the keyserver to fetch the farcaster channel info for the given farcasterChannelID, and uses that info to display the farcaster channel name if their is an existing tag. Otherwise it will let the user know that there are currently no tagged farcaster channels tagged to that community.

The mechanism for getting the farcaster channel info that I came up with is a bit naive and not very performant since we are fetching the channel info pretty often; however, I had to make sure I didn't blow up the scope of this project, and I felt that the tradeoff to ship this faster over slightly improved performance was necessary. If this implementation is okay, I will also create a linear task before landing to track making this mechanism more performant.

Depends on D12026

Test Plan

Please see demo video below (Remove button will be introduced in a separate diff)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.May 15 2024, 1:04 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptMay 15 2024, 1:04 AM

ginsu edited the summary of this revision. (Show Details)May 15 2024, 1:15 AM

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

ginsu added reviewers: tomek, inka.

ginsu attached a referenced file: F1765901: Screen Recording 2024-05-15 at 4.14.13 AM.mov. (Show Details)

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

ginsu added a parent revision: D12027: [lib] update community reducer to handle new community action types.

ginsu edited the summary of this revision. (Show Details)May 15 2024, 1:17 AM

ginsu added a reviewer: ashoat.

ginsu added a parent revision: D12026: [lib] introduce fetchFarcasterChannelInfo to neynar client class.

ginsu removed a parent revision: D12027: [lib] update community reducer to handle new community action types.

Harbormaster completed remote builds in B28927: Diff 40201.May 15 2024, 1:21 AM

ginsu requested review of this revision.May 15 2024, 1:21 AM

Making @ashoat a blocking reviewer to review copy

native/community-settings/tag-farcaster-channel/tag-farcaster-channel.react.js
83 ↗	(On Diff #40201)	This copy has already been reviewed in a previous diff and is just being reintroduced since I temporarily removed it in a previous diff https://github.com/CommE2E/comm/blob/master/native/community-settings/tag-farcaster-channel/tag-farcaster-channel.react.js#L167
95 ↗	(On Diff #40201)	This copy needs to be reviewed

ginsu edited the summary of this revision. (Show Details)May 15 2024, 1:24 AM

ginsu mentioned this in D12042: [native] extend loading logic to include fetching farcaster channel info.May 15 2024, 1:25 AM

ginsu added a child revision: D12042: [native] extend loading logic to include fetching farcaster channel info.May 15 2024, 1:28 AM

Could the "Tag Farcaster channel" button say "Farcaster channel" in case the channel is already tagged?

native/community-settings/tag-farcaster-channel/tag-farcaster-channel.react.js
49–63 ↗	(On Diff #40201)	Why do we get channel info in params if we fetch it here anyway?

The displayed name of the channel looks like some text, not like a name. Could we maybe display the Farcaster icon next to it or something?

ashoat requested changes to this revision.May 15 2024, 5:19 PM

ashoat added inline comments.

native/community-settings/tag-farcaster-channel/tag-farcaster-channel.react.js
49–63 ↗	(On Diff #40201)	It looks like the parent component is trying to pre-load the data. By passing it in the navigation params, we allow this component to immediately display the data. The loading here is necessary if the pre-loading did not complete in time, or if some of the input data (`communityInfo?.farcasterChannelID` or `fid`) change. We could add a performance optimization to make it so it's not re-fetched on the very first load of this component if the pre-load worked, but then it is still re-fetched in any inputs changed. But I'm not sure how valuable that would be
82 ↗	(On Diff #40201)	Let's avoid using `selectedChannel.name` anywhere in the UI. Instead, let's use `/${selectedChannel.id}` everywhere
83 ↗	(On Diff #40201)	Can we maybe not bold it when there's no Farcaster channel tagged?

This revision now requires changes to proceed.May 15 2024, 5:19 PM

Could the "Tag Farcaster channel" button say "Farcaster channel" in case the channel is already tagged?

If the Comm community is already tagged, then we'll see a delete button. @ginsu do we have a way to know if the Farcaster channel is already associated with a Comm community? (I recall that it's 1-to-1)

The displayed name of the channel looks like some text, not like a name. Could we maybe display the Farcaster icon next to it or something?

I agree it's ambiguous, but I don't think a Farcaster icon is necessary if we implement my inline suggestion to use eg. /memes instead of Memes.

address comments

@ginsu do we have a way to know if the Farcaster channel is already associated with a Comm community? (I recall that it's 1-to-1)

No this is out of scope for this project, but is tracked here: https://linear.app/comm/issue/ENG-8032/filter-list-of-taken-farcaster-channels

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

ginsu attached a referenced file: F1771871: Screen Recording 2024-05-15 at 9.02.42 PM.mov. (Show Details)

Harbormaster completed remote builds in B28964: Diff 40261.May 15 2024, 6:15 PM

What happens if a user attempts to tag a community to a Farcaster channel that already has a community tagged to it?

What happens if a user attempts to tag a community to a Farcaster channel that already has a community tagged to it?

We show an error message which was introduced in D11809

ashoat accepted this revision.May 15 2024, 7:11 PM

ashoat added inline comments.

native/community-settings/tag-farcaster-channel/tag-farcaster-channel.react.js
126 ↗	(On Diff #40261)	Why does the `height` need to be hardcoded here?

This revision is now accepted and ready to land.May 15 2024, 7:11 PM

ginsu added inline comments.Jun 7 2024, 4:19 PM

native/community-settings/tag-farcaster-channel/tag-farcaster-channel.react.js
126 ↗	(On Diff #40261)	Since the channel name text is bolded and the "No Farcaster channel tagged" text is not bolded, there was a slight difference in height and this resulted in the "Tag channel" button moving slightly by a pixel or two. Hardcoding the height to match the height of when the channel name text is bolded fixes this issue