-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FormTokenField: Added Padding to resolve close button overlap issue #41556
FormTokenField: Added Padding to resolve close button overlap issue #41556
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch!
I can see the CSS structure is not in an optimal place to begin with. For example, the button outline is already too close to the text in the default variant, and starts to overlap text in the borderless variant. And the default/borderless switching really shouldn't be this hacky.
CleanShot.2022-06-06.at.20.56.08.mp4
But I understand that a proper fix requires a bit more in-depth reworking of the elements. So I'm fine with merging the bandaid fix in this PR for now, as it is at least incrementally better and doesn't add any complexity that would be hard to remove when refactoring.
I'd just like to rope in @jameskoster for a quick design check on the spacings. (Bonus: James, it would be cool if you could give us some updated mockups of this component so the button isn't jammed and has the newer look & feel 🙂 Not necessary for this PR though.)
Here's what I see: To my eye the Agree this component could use some design love. I'm not sure about the history, but I don't know that we really need the 'borderless' variant, there's probably more value in a consistent appearance? It's especially strange since neither variant has a border :) |
@sunil25393 Thanks for updating! If you could add a brief changelog entry, this PR is ready to merge 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 🚀
What?
Fixed issue of close button overlap while isBorderless prop is true in FormTokenField component.
Why?
FIx #41447
Testing Instructions
Please include step by step instructions on how to test this PR.
Screenshots or screencast