FormControl refactor/cleanup #2114
Conversation
🦋 Changeset detectedLatest commit: d3c516c 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 |
src/forms/FormControl.scss
Outdated
| &.FormControl-fieldWrap--input-trailingAction { | ||
| grid-template-columns: var(--base-size-16, 16px) minmax(0, auto) min-content; | ||
| // show vertical divider line between field and button | ||
| &::before { |
There was a problem hiding this comment.
Should this divider line be an optional prop?
There was a problem hiding this comment.
Yes, if you can describe when the line should/should not be used.
Also: we don't have this style as an option in PRC. Is this to support a legacy style? Or is this a current pattern we need to support?
There was a problem hiding this comment.
This came from @vdepizzol design for the new AppHeader, so a new pattern. We were thinking it might make sense to recommend it for fields that have a secondary action, not a clear button as you see in my screenshot. We can help address this in the form interface guidelines once this ships!
There was a problem hiding this comment.
So if this isn't a clear button, what does it do?
docs/src/stories/components/Experimental/Forms/Checkbox.stories.jsx
Outdated
Show resolved
Hide resolved
docs/src/stories/components/Experimental/Forms/Checkbox.stories.jsx
Outdated
Show resolved
Hide resolved
docs/src/stories/components/Experimental/Forms/Checkbox.stories.jsx
Outdated
Show resolved
Hide resolved
docs/src/stories/components/Experimental/Forms/Radio.stories.jsx
Outdated
Show resolved
Hide resolved
| } | ||
| // extend touch target | ||
| &::after { | ||
| @include minTouchTarget(var(--primer-control-medium-size, 32px), var(--primer-control-medium-size, 32px)); |
There was a problem hiding this comment.
@mperrotti I extended the touch target to handle that gap between label/input (and to extend a bit beyond the tiny input) 🌸
|
Windows High Contrast mode testing from @ichelsea:
|
| @@ -96,8 +96,6 @@ button, | |||
| [role='button'], | |||
| input[type='radio'], | |||
| input[type='checkbox'] { | |||
| transition: 80ms cubic-bezier(0.33, 1, 0.68, 1); | |||
| transition-property: color, background-color, box-shadow, border-color; | |||
There was a problem hiding this comment.
This might fix a bunch of the transition bugs we've seen? 🤩
There was a problem hiding this comment.
I think so.. I feel a little worried deleting this but its probably fine? Just a transition 😅
There was a problem hiding this comment.
I don't believe there was functionality relying on this
What are you trying to accomplish?
Slight refactor and simplification for all Form components (based off the new app header styles).
position: absoluteand specific inline padding for fields.This will correspond with the new Rails forms framework, but I added some stories for easier visual testing.
Can these changes ship as is?
This markup hasn't been released yet in PVC/dotcom, so we're safe to make breaking changes.
TODO: