fix(form-control): add transition styles - #8211
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe form control stylesheet adds configurable transition properties, applies them to the control and its pseudo-elements, and changes disabled pseudo-elements to transparent borders while preserving the disabled cursor. ChangesForm control transitions
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview: https://pf-pr-8211.surge.sh A11y report: https://pf-pr-8211-a11y.surge.sh |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patternfly/components/FormControl/form-control.scss`:
- Around line 21-24: The new custom property
--#{$form-control}--TransitionDuration is introduced but not used everywhere
(the duration is still hardcoded/bypassed in the other rule), so update the
rules that set transition-duration to instead reference
var(--#{$form-control}--TransitionDuration); locate occurrences of
--#{$form-control}--TransitionDuration and replace any direct/two-token duration
usages (the block around the other transition declaration) so all transition
declarations use var(--#{$form-control}--TransitionDuration) as the single
source of truth (also update the duplicate occurrences noted around the 168-171
block).
- Line 454: The icon's transition is inheriting from
.pf-v6-c-form-control__utilities (which has no transition) causing snap changes;
update the stylesheet by either adding "transition: inherit;" to the
.pf-v6-c-form-control__utilities rule or by giving .pf-v6-c-form-control__icon
explicit transition longhand properties (e.g.,
transition-property/transition-duration/transition-timing-function) so the
.pf-v6-c-form-control__icon animates consistently with the root
.pf-v6-c-form-control.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97b24b9f-5e90-45e6-a94d-9b2919332057
📒 Files selected for processing (1)
src/patternfly/components/FormControl/form-control.scss
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/patternfly/components/FormControl/form-control.scss (1)
309-315:⚠️ Potential issue | 🟠 MajorKeep the disabled border animatable.
The new transition only helps if the pseudo-borders stay present. Lines 314-315 switch both of them to
none, so the outline still disappears instantly when.pf-m-disabledtoggles. That leaves the field border snapping while adjacentpf-m-controlbuttons fade. Use transparent border colors here instead of removing the border style.♻️ Proposed fix
&.pf-m-disabled { --#{$form-control}--BackgroundColor: var(--#{$form-control}--m-disabled--BackgroundColor); --#{$form-control}--Color: var(--#{$form-control}--m-disabled--Color); --#{$form-control}--m-placeholder--Color: var(--#{$form-control}--m-disabled--Color); --#{$form-control}__toggle-icon--Color: var(--#{$form-control}--m-disabled__toggle-icon--Color); - --#{$form-control}--before--BorderStyle: none; - --#{$form-control}--after--BorderStyle: none; + --#{$form-control}--before--BorderColor: transparent; + --#{$form-control}--after--BorderColor: var(--#{$form-control}--m-disabled--after--BorderColor); cursor: not-allowed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/components/FormControl/form-control.scss` around lines 309 - 315, The disabled state currently sets --#{$form-control}--before--BorderStyle and --#{$form-control}--after--BorderStyle to none which removes the pseudo-borders and prevents the border transition; instead keep the border style (don’t set them to none) and set the corresponding border-color variables to the disabled/transparent values so the outline can animate (update usage around .pf-m-disabled and the CSS vars --#{$form-control}--before--BorderStyle, --#{$form-control}--after--BorderStyle and the matching --#{$form-control}--before--BorderColor / --#{$form-control}--after--BorderColor or their --m-disabled counterparts).
♻️ Duplicate comments (1)
src/patternfly/components/FormControl/form-control.scss (1)
25-28:⚠️ Potential issue | 🟡 MinorThe new child transition hooks are still effectively disabled.
__control,__icon, and__toggle-iconall default tonone/0s, and I don't see any override in this file that binds them back to the wrapper transition. That means the shell will animate, but child pieces that still paint their own state — for example theselectbackground on Line 234 and the disabled toggle-icon color on Line 313 — will still snap.Also applies to: 141-164, 219-222, 469-491
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/components/FormControl/form-control.scss` around lines 25 - 28, The child transition custom properties for the form control components (e.g., --#{$form-control}__control--TransitionDuration, --#{$form-control}__control--TransitionProperty, and their __icon and __toggle-icon counterparts) are set to 0s/none which disables child animations; update these child variables to inherit or reference the wrapper transition variables (the wrapper's transition timing/duration/property variables) so child parts (select background, disabled toggle-icon color, etc.) follow the wrapper animation, and remove hard-coded 0s/none defaults in the blocks that define __control, __icon, and __toggle-icon (also apply same change to the other occurrences covering the ranges noted) so children animate smoothly with the wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patternfly/components/Button/button.scss`:
- Around line 942-945: The background shorthand currently always composes
disabled background with --#{$button}--BackgroundSurface which forces a tint
onto transparent variants like --#{$button}--m-plain--disabled--BackgroundColor:
transparent; change the rule that sets background (the line using
var(--#{$button}--disabled--BackgroundColor)
var(--#{$button}--BackgroundSurface)) so the surface is overridable—e.g. use the
surface custom property with a fallback of none or conditionally omit it—so
plain/transparent disabled buttons can set --#{$button}--BackgroundSurface: none
(or leave it unset) and stay transparent; update the background declaration
referencing --#{$button}--disabled--BackgroundColor and
--#{$button}--BackgroundSurface accordingly.
---
Outside diff comments:
In `@src/patternfly/components/FormControl/form-control.scss`:
- Around line 309-315: The disabled state currently sets
--#{$form-control}--before--BorderStyle and
--#{$form-control}--after--BorderStyle to none which removes the pseudo-borders
and prevents the border transition; instead keep the border style (don’t set
them to none) and set the corresponding border-color variables to the
disabled/transparent values so the outline can animate (update usage around
.pf-m-disabled and the CSS vars --#{$form-control}--before--BorderStyle,
--#{$form-control}--after--BorderStyle and the matching
--#{$form-control}--before--BorderColor / --#{$form-control}--after--BorderColor
or their --m-disabled counterparts).
---
Duplicate comments:
In `@src/patternfly/components/FormControl/form-control.scss`:
- Around line 25-28: The child transition custom properties for the form control
components (e.g., --#{$form-control}__control--TransitionDuration,
--#{$form-control}__control--TransitionProperty, and their __icon and
__toggle-icon counterparts) are set to 0s/none which disables child animations;
update these child variables to inherit or reference the wrapper transition
variables (the wrapper's transition timing/duration/property variables) so child
parts (select background, disabled toggle-icon color, etc.) follow the wrapper
animation, and remove hard-coded 0s/none defaults in the blocks that define
__control, __icon, and __toggle-icon (also apply same change to the other
occurrences covering the ranges noted) so children animate smoothly with the
wrapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3c35893-4680-434d-af6f-26dd7f33c25b
📒 Files selected for processing (2)
src/patternfly/components/Button/button.scsssrc/patternfly/components/FormControl/form-control.scss
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/patternfly/components/FormControl/form-control.scss (1)
25-28:⚠️ Potential issue | 🟡 MinorPropagate real transition values to the nested control/icon tokens.
These defaults still resolve the child longhands to
0s / none, so the consumers at Lines 220-223 and Lines 467-492 will snap while the wrapper animates.autoalso is not a validtransition-timing-function, so these tokens will not behave as written once a non-zero transition is assigned.♻️ Proposed fix
- --#{$form-control}__control--TransitionDelay: 0s; - --#{$form-control}__control--TransitionTimingFunction: auto; - --#{$form-control}__control--TransitionDuration: 0s; - --#{$form-control}__control--TransitionProperty: none; + --#{$form-control}__control--TransitionDelay: var(--#{$form-control}--TransitionDelay); + --#{$form-control}__control--TransitionTimingFunction: var(--#{$form-control}--TransitionTimingFunction); + --#{$form-control}__control--TransitionDuration: var(--#{$form-control}--TransitionDuration); + --#{$form-control}__control--TransitionProperty: color, background-color; ... - --#{$form-control}__icon--TransitionDelay: 0s; - --#{$form-control}__icon--TransitionTimingFunction: auto; - --#{$form-control}__icon--TransitionDuration: 0s; - --#{$form-control}__icon--TransitionProperty: none; + --#{$form-control}__icon--TransitionDelay: var(--#{$form-control}--TransitionDelay); + --#{$form-control}__icon--TransitionTimingFunction: var(--#{$form-control}--TransitionTimingFunction); + --#{$form-control}__icon--TransitionDuration: var(--#{$form-control}--TransitionDuration); + --#{$form-control}__icon--TransitionProperty: color; ... - --#{$form-control}__toggle-icon--TransitionDelay: 0s; - --#{$form-control}__toggle-icon--TransitionTimingFunction: auto; - --#{$form-control}__toggle-icon--TransitionDuration: 0s; - --#{$form-control}__toggle-icon--TransitionProperty: none; + --#{$form-control}__toggle-icon--TransitionDelay: var(--#{$form-control}--TransitionDelay); + --#{$form-control}__toggle-icon--TransitionTimingFunction: var(--#{$form-control}--TransitionTimingFunction); + --#{$form-control}__toggle-icon--TransitionDuration: var(--#{$form-control}--TransitionDuration); + --#{$form-control}__toggle-icon--TransitionProperty: color;Is `auto` a valid value for CSS `transition-timing-function`? Also, when a CSS custom property makes a `transition-*` longhand invalid at computed-value time, does the property fall back to its initial value?Also applies to: 142-145, 162-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/components/FormControl/form-control.scss` around lines 25 - 28, The nested control/icon transition token defaults are invalid/too-zeroed (e.g., "--#{$form-control}__control--TransitionTimingFunction: auto" and 0s/none for delay/duration/property) so child longhands snap; change the nested tokens in form-control.scss to reference the wrapper's real transition tokens (e.g., replace "--#{$form-control}__control--TransitionDelay", "--#{$form-control}__control--TransitionTimingFunction", "--#{$form-control}__control--TransitionDuration", "--#{$form-control}__control--TransitionProperty" with var(...) lookups of the parent tokens like "--#{$form-control}--TransitionDelay", "--#{$form-control}--TransitionTimingFunction" etc., and use a valid default timing-function (e.g., ease) or a fallback in the var(...) so computed values remain valid); apply the same change pattern to the other occurrences noted (around lines 142-145 and 162-165) so child controls inherit/propagate the wrapper's real transition values instead of 0s/none or the invalid "auto".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/patternfly/components/FormControl/form-control.scss`:
- Around line 25-28: The nested control/icon transition token defaults are
invalid/too-zeroed (e.g.,
"--#{$form-control}__control--TransitionTimingFunction: auto" and 0s/none for
delay/duration/property) so child longhands snap; change the nested tokens in
form-control.scss to reference the wrapper's real transition tokens (e.g.,
replace "--#{$form-control}__control--TransitionDelay",
"--#{$form-control}__control--TransitionTimingFunction",
"--#{$form-control}__control--TransitionDuration",
"--#{$form-control}__control--TransitionProperty" with var(...) lookups of the
parent tokens like "--#{$form-control}--TransitionDelay",
"--#{$form-control}--TransitionTimingFunction" etc., and use a valid default
timing-function (e.g., ease) or a fallback in the var(...) so computed values
remain valid); apply the same change pattern to the other occurrences noted
(around lines 142-145 and 162-165) so child controls inherit/propagate the
wrapper's real transition values instead of 0s/none or the invalid "auto".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: eea76722-b741-4da4-81b9-25bd1464f712
📒 Files selected for processing (2)
src/patternfly/components/Button/button.scsssrc/patternfly/components/FormControl/form-control.scss
|
This PR has been automatically marked as stale because it has not had activity in the last 60 days. |
Address review feedback by handling the disabled animation sync entirely from form-control.scss: - Revert button.scss; the wrapper transition is sufficient. - Drop the __control, __icon and __toggle-icon transition tokens and the places they were applied. Icons are out of scope here. - Drop `transition: inherit` from the ::before/::after and __utilities rules. - Restore `--before--BorderStyle: none` / `--after--BorderStyle: none` on the disabled modifier and remove the associated border color token. The wrapper keeps the same transition properties and duration the buttons use, so the input background and border stay in sync with the increment/decrement buttons.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/patternfly/components/FormControl/form-control.scss (1)
168-171: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply the transition to the pseudo-elements that render the border.
The transition timing is only on
.pf-v6-c-form-control; its::beforeand::afterinherit the CSS variables but not the transition properties, soborder-colorchanges on those pseudo-elements snap. Add the transition timing to&::before, &::after, and avoid animating the disabled-stateborder-styledisappearance since changingborder-styletononeis discrete.Proposed fix
&::before, &::after { position: absolute; inset: 0; pointer-events: none; content: ""; + transition-delay: var(--#{$form-control}--TransitionDelay); + , + transition-timing-function: var(--#{$form-control}--TransitionTimingFunction); + transition-duration: var(--#{$form-control}--TransitionDuration); + transition-property: border-color; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/patternfly/components/FormControl/form-control.scss` around lines 168 - 171, Update the transition declarations in the form-control styles so they also apply to the border-rendering `&::before` and `&::after` pseudo-elements, allowing their `border-color` changes to animate. Exclude `border-style` from the transition to prevent animating the disabled-state switch to `none`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/patternfly/components/FormControl/form-control.scss`:
- Around line 168-171: Update the transition declarations in the form-control
styles so they also apply to the border-rendering `&::before` and `&::after`
pseudo-elements, allowing their `border-color` changes to animate. Exclude
`border-style` from the transition to prevent animating the disabled-state
switch to `none`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 89ed4b34-b045-4f08-9ded-572b3a327354
📒 Files selected for processing (1)
src/patternfly/components/FormControl/form-control.scss
mcoker
left a comment
There was a problem hiding this comment.
Thanks for making those updates! This looks great except for one thing. The borders (on ::before/::after) are not syncing with the button.
You can probably just do something like this
- Add
transition: inherit;to https://github.com/tarunvashishth/patternfly/blob/fa2a6f8558891baadbac3c479297becb8fc7874e/src/patternfly/components/FormControl/form-control.scss#L165-L171 - Remove the vars that set the border-styles to none when disabled https://github.com/tarunvashishth/patternfly/blob/fa2a6f8558891baadbac3c479297becb8fc7874e/src/patternfly/components/FormControl/form-control.scss#L289-L290
- In that same
.pf-m-disabledblock from ^, add&:is(::before, ::after) { border-color: transparent; }
Inherit the wrapper transition on ::before/::after so their border-color animates, and keep the borders present when disabled by making them transparent instead of removing the border style.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/patternfly/components/FormControl/form-control.scss`:
- Around line 301-304: Update the disabled-state &::before and &::after rules in
the form-control styles to use the existing
--#{$form-control}--m-disabled--after--BorderColor token rather than hardcoding
transparent, preserving consumer overrides for both pseudo-elements; remove the
token only if it is intentionally no longer needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 515ba278-9792-41ce-82fc-68c4b7e822b2
📒 Files selected for processing (1)
src/patternfly/components/FormControl/form-control.scss
mcoker
left a comment
There was a problem hiding this comment.
Perfect, thank you! Visual regressions passed and number input looks good.
numberinputdisabled.mov
|
🎉 This PR is included in version 6.6.0-prerelease.26 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Add transition to form-control so the input background and border changes animate in sync with the increment/decrement buttons when toggling isDisabled.
This resolves the visual delay where the buttons and input updated at different times.
Screen.Recording.2026-03-07.at.7.18.45.PM.mov
Fixes #7429
Summary by CodeRabbit