This diff introduces the RemoveTagButton. This button is used to remove a farcaster channel tag from a community
Depends on D12530
Details
Details
- Reviewers
inka kamil ashoat - Commits
- rCOMM27d7212e41e4: [lib/native/web] introduce RemoveTagButton
Confirmed that when I clicked on the button the tag was removed
w/o error message:
w/ error message:
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| web/tag-farcaster-channel/tag-farcaster-channel-modal.react.js | ||
|---|---|---|
| 52–53 ↗ | (On Diff #41593) | A subsequent diff will handle this |
Comment Actions
The space under the channel name looks awkward without the error to me (seems too big). Is there any way we can change this? I can see in invite links modal, that when an error appears we change the size of the modal to add the additional needed space. Could that work?
Alternatively I think the space could just be smaller, similarly to how we display errors on the login screen:
What do you think?
Comment Actions
I can see in invite links modal, that when an error appears we change the size of the modal to add the additional needed space
I have gotten feedback in the past that this we actually should never change the size of the modal to add additional needed space; however, your second suggestion about making the space a bit smaller should work!
Comment Actions
I am not reviewing this because I assumed that you plan to introduce the changes we discussed. Is this incorrect?
Comment Actions
Passing back to @ginsu to "make the space smaller"
I can see in invite links modal, that when an error appears we change the size of the modal to add the additional needed space. Could that work?
This is a huge antipattern, and it's disappointing it got landed so recently. cc @tomek: modal dimensions should NEVER resize like this.
In general, I encourage everybody on the team to think more critically about UX. We don't have a designer on the team, which means we rely on developers to be thoughtful about UX. Having a modal that resizes on error (or really for any other reason) is a really, really bad UX... it's jarring, all of the buttons move, etc.
I hope such a design is never recommended, considered, or implemented on this team ever again.
I've found that more than anybody else on the team, you have a pattern or leaving comments without actions (accept or request changes). I think it would be more clear if you included actions more often. We've talked about this in retro and generally the only reason we'd want to avoid it is if there is somebody else on the reviewers list that you think should still have the diff in their review queue. I don't think that's the case here.
| web/tag-farcaster-channel/tag-farcaster-channel-modal.css | ||
|---|---|---|
| 28 ↗ | (On Diff #41593) | Why do we need to hardcode height here? |
| web/tag-farcaster-channel/tag-farcaster-channel-modal.css | ||
|---|---|---|
| 28 ↗ | (On Diff #41593) | 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 |
| web/tag-farcaster-channel/tag-farcaster-channel-modal.css | ||
|---|---|---|
| 27 ↗ | (On Diff #41818) | Reduced the space between the between the channel name and error message by 50% (8px => 4px) |
Comment Actions
I think it still looks bad. Can we try to decrease the space here? We can use position: absolute to get it to appear where we want
| web/tag-farcaster-channel/tag-farcaster-channel-modal.css | ||
|---|---|---|
| 27 ↗ | (On Diff #41818) | Let's position this absolutely |