-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix(style): synced text field styles with pattern library #1206
Conversation
Preview branch generated at https://fix-sync-text-field-with-uxpin.d1gko6en628vir.amplifyapp.com |
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.
A couple of things here:
The error field focus ring shifts content during focus (see below video), the footprint of the element should not be shifting content on the page:
cauldron-error-textfield-focus.mov
The default focus ring still appears to be too large, according to our pattern library it needs to be 1px smaller.
Minor nit: the commit message / pr title should be more descriptive in describing what changed. Something like "synced text field styles with pattern library". Otherwise, it will be difficult to know what actually changed when it gets placed in the changelog.
We need to also ensure that any style changes are reflected on Combobox as well. Combobox isn't a 1:1 of textfield and has to duplicate a few styles over as it cannot use the textfield styles directly.
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.
Changes here look good for the textfield/combobox error focus ring changes.
It does look like we're still missing the following from the issue:
- Focus ring should be 1px smaller (currently appears to be 3px)
The above applies to the default "blue" focus ring that gets applied, so we just need to shrink both TextField and Combobox by 1px.
This is also an existing issue, but I noticed that hover state of the combobox error is currently using the wrong focus color that would be nice to sync up here as well:
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.
One remaining nit, and this should be good to go!
This pull request is automatically being deployed by Amplify Hosting (learn more). |
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.
Closes: #1070