fix(form-control): add transition styles#8211
Open
tarunvashishth wants to merge 3 commits into
Open
Conversation
Contributor
WalkthroughAdds transition custom properties and propagation for FormControl elements and introduces a two-layer button background via a new BackgroundSurface CSS variable, with disabled state updates to preserve the surface layer. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Collaborator
|
Preview: https://pf-pr-8211.surge.sh A11y report: https://pf-pr-8211-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Contributor
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
Contributor
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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