Page MenuHomePhabricator

[web] introduce tagFarcasterChannelByName to CreateFarcasterChannelTagModal
ClosedPublic

Authored by ginsu on Jun 24 2024, 11:01 PM.
Tags
None
Referenced Files
F3582103: D12567.diff
Sun, Dec 29, 10:54 AM
Unknown Object (File)
Sat, Dec 28, 9:31 AM
Unknown Object (File)
Sat, Dec 28, 9:31 AM
Unknown Object (File)
Sat, Dec 28, 9:31 AM
Unknown Object (File)
Sat, Dec 28, 9:31 AM
Unknown Object (File)
Fri, Dec 13, 10:06 AM
Unknown Object (File)
Wed, Dec 11, 1:35 PM
Unknown Object (File)
Mon, Dec 9, 11:55 PM
Subscribers

Details

Summary

This diff introduces the "Other" option in the CreateFarcasterChannelTagModal dropdown options. Just like we do in native if the user selects this option, we prompt the user with a text input where they can manually input the name of the farcaster channel to create the tag.

I know the large amount of white space does not look super great; however, it is needed so that the size of the modal does not change when the text input is rendered on the modal. Open to other suggestions tho, if anyone has any better ideas

Depends on D12566

Test Plan

Please see the demo video

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: inka, kamil.
ginsu added a reviewer: ashoat.

This diff contains copy that needs to be reviewed

web/tag-farcaster-channel/create-farcaster-channel-tag-modal.react.js
81 ↗(On Diff #41675)

This copy needs to be reviewed

inka added inline comments.
web/tag-farcaster-channel/create-farcaster-channel-tag-modal.react.js
41 ↗(On Diff #41675)

We typically call those x and setX. I would rather we kept to that because it makes the code easier to read

This revision is now accepted and ready to land.Jun 27 2024, 6:49 AM

Making @ashoat a blocking reviewer since there is copy to review

This revision now requires review to proceed.Jun 27 2024, 6:50 AM
web/tag-farcaster-channel/create-farcaster-channel-tag-modal.react.js
41 ↗(On Diff #41675)

Thanks for catching that, don't know what I was thinking...

ashoat requested changes to this revision.Jun 27 2024, 1:11 PM

Would be good if it weren't for the CSS hack

web/tag-farcaster-channel/create-farcaster-channel-tag-modal.css
31 ↗(On Diff #41675)

Hardcoding height like this is extremely hacky and an anti-pattern.

Instead, can you please keep:

  1. Make sure that stuff inside tagFarcasterChannelByName is always rendered
  2. However, when selectedOption !== 'other', you can render it invisibly. I believe this can be done with visibility: hidden. That way, the appropriate amount of height for that stuff will be taken up, without you having to guess numbers that may or may not work across all browsers
  3. Please review the rest of your stack for ANY and ALL hard-coded height, and replace it with this approach
web/tag-farcaster-channel/create-farcaster-channel-tag-modal.react.js
100–116 ↗(On Diff #41675)

Let's reorder this to reduce the number of branches the reader has to keep in their "mental RAM"

if (!selectedOption) {
  return;
}

if (selectedOption !== 'other') {
  createTag(selectedOption);
  return;
}

const channelInfo =
  await neynarClientContext.client.fetchFarcasterChannelByName(
    channelNameText,
  );
if (!channelInfo) {
  setError('channel_not_found');
  return;
}
createTag(channelInfo.id);
This revision now requires changes to proceed.Jun 27 2024, 1:11 PM
This revision is now accepted and ready to land.Jul 1 2024, 4:16 PM
This revision was landed with ongoing or failed builds.Jul 2 2024, 1:44 PM
This revision was automatically updated to reflect the committed changes.