-
Notifications
You must be signed in to change notification settings - Fork 1.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
FormControl + Autocomplete component updates #2075
Conversation
🦋 Changeset detectedLatest commit: 81510ca The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…l-component-updates
…rimer/css into form-control-component-updates
src/forms/FormControl.scss
Outdated
// stylelint-disable primer/typography, primer/borders, primer/spacing, primer/box-shadow, max-nesting-depth | ||
|
||
:root { | ||
accent-color: var(--color-accent-fg); |
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.
This seems a bit general. Should we focus it to the selector using it?
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.
I think it only applies to a few input
elements by default https://developer.mozilla.org/en-US/docs/Web/CSS/accent-color
BUT, I removed it anyways. We're adding custom Checkbox/Radio styles instead of relying on browser defaults going forward.
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.
If we wanted to add it for all "native" controls, maybe native.scss
would be a good place. It's where we have the color-scheme
and Windows High Contrast mode styles.
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.
Just based on the PVC docs, looks good.
The only small tweak I found is nudging the clear button 2px
to the left?
And I assume there will be more follow-up changes when implementing the other FormControl
s in PVC.
- title: Autocomplete | ||
url: /components/autocomplete |
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.
👍 On just removing the nav item for now.
Internal discussion around not having to maintain documentation in PCSS AND PVC.
pointer-events: none; | ||
content: ''; | ||
background-color: var(--color-fg-muted); | ||
mask: url('data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iMTYiIGhlaWdodD0iMTYiIHZpZXdCb3g9IjAgMCAxNiAxNiIgZmlsbD0iIzU4NjA2OSIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIj48cGF0aCBkPSJNNC40MjcgOS40MjdsMy4zOTYgMy4zOTZhLjI1MS4yNTEgMCAwMC4zNTQgMGwzLjM5Ni0zLjM5NkEuMjUuMjUgMCAwMDExLjM5NiA5SDQuNjA0YS4yNS4yNSAwIDAwLS4xNzcuNDI3ek00LjQyMyA2LjQ3TDcuODIgMy4wNzJhLjI1LjI1IDAgMDEuMzU0IDBMMTEuNTcgNi40N2EuMjUuMjUgMCAwMS0uMTc3LjQyN0g0LjZhLjI1LjI1IDAgMDEtLjE3Ny0uNDI3eiIgLz48L3N2Zz4='); |
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.
Is browser support ok for mask
? Not sure if it's https://caniuse.com/css-masks. autoprefixer might add -webkit-
?
What are you trying to accomplish?
While working on bringing the PVC Autocomplete component up to design specs, I noticed we had some custom CSS specific for Autocomplete rather than sharing more with generic form styles. I worked on aligning props in Autocomplete with PRC, and built some prototypes for how the markup would look for all form components (input, select, checkbox to start).
What should reviewers focus on?
This will immediately be used for Autocomplete, and other form components are still in progress. For this PR, I would focus on text-input type styles specifically.
For now, testing can be done in the PVC PR: primer/view_components#1163
Can these changes ship as is?
Remaining things to do: