feat(react-checkbox): add motion to Checkbox #35380
Open
Conversation
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
|
Pull request demo site: URL |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
change/@fluentui-react-checkbox-efe8019d-5ed0-4eec-ae6d-674f97612c48.json
Show resolved
Hide resolved
📊 Bundle size report
Unchanged fixtures
|
dmytrokirpa
reviewed
Oct 24, 2025
packages/react-components/react-checkbox/library/etc/react-checkbox.api.md
Show resolved
Hide resolved
dmytrokirpa
reviewed
Oct 24, 2025
| }, | ||
| }, | ||
|
|
||
| unchecked: {}, |
There was a problem hiding this comment.
why do we need empty style rule?
| // Validate visual state by checking computed opacity (0 -> unchecked, 1 -> checked) | ||
| cy.get(`[data-test="list-item-${index + 1}"] .fui-Checkbox__indicator > svg`).should($svg => { | ||
| // Read computed opacity and coerce to number for robust comparison | ||
| const opacity = Number(window.getComputedStyle($svg[0]).opacity); |
There was a problem hiding this comment.
It should probably be aria-hidden or some other data-* attribute, we shouldn't rely on computed styles in tests
| }, | ||
| elementType: 'div', | ||
| }), | ||
| checkmarkIcon: slot.optional(props.checkmarkIcon, { |
There was a problem hiding this comment.
I maybe missing something, but what's the main idea behind having this as a separate slot?
|
For some reason checkmark icon doesn't render on PR docsite preview https://fluentuipr.z22.web.core.windows.net/pull/35380/public-docsite-v9/react/index.html?path=/docs/components-checkbox--docs#default Screen.Recording.2025-10-24.at.15.30.55.mov |
mainframev
requested changes
Oct 24, 2025
| ); | ||
| // Checkbox now keeps the checkmark SVG in the DOM for motion/animation. | ||
| // Validate visual state by checking computed opacity (0 -> unchecked, 1 -> checked) | ||
| cy.get(`[data-test="list-item-${index + 1}"] .fui-Checkbox__indicator > svg`).should($svg => { |
There was a problem hiding this comment.
can you import and interpolate css checkbox className here instead of hardcoded string?
Comment on lines
+297
to
+305
| checked === 'mixed' ? rootStyles.mixed : checked ? rootStyles.checked : rootStyles.unchecked, | ||
| checked === 'mixed' ? motionScaleStyles.mixed : checked ? motionScaleStyles.checked : motionScaleStyles.unchecked, | ||
| !disabled && | ||
| (checked === 'mixed' | ||
| ? motionColorStyles.mixed | ||
| : checked | ||
| ? motionColorStyles.checked | ||
| : motionColorStyles.unchecked), | ||
| disabled && rootStyles.disabled, |
There was a problem hiding this comment.
I would avoid nested ternaries, looks shorter, but harder to read (https://eslint.org/docs/latest/rules/no-nested-ternary)
consider to abstract this to a function, something like that (just an example):
type checkState = 'checked' | 'unchecked' | 'mixed';
const checkMap: Record<boolean | 'mixed', checkState> = {
true: 'checked',
false: 'unchecked',
mixed: 'mixed',
};
const collectCheckStateClasses = (
checked: boolean | 'mixed',
disabled: boolean,
rootStyles: ReturnType<typeof useRootStyles>,
motionScaleStyles: ReturnType<typeof useMotionScaleStyles>,
motionColorStyles: ReturnType<typeof useMotionColorStyles>,
) => {
const state = checkMap[checked];
return [
rootStyles[state],
motionScaleStyles[state],
!disabled && motionColorStyles[state],
].filter(Boolean);
};
mainframev
reviewed
Oct 24, 2025
| 'use client'; | ||
|
|
||
| import { makeResetStyles, makeStyles, mergeClasses } from '@griffel/react'; | ||
| import { GriffelStyle, makeResetStyles, makeStyles, mergeClasses } from '@griffel/react'; |
There was a problem hiding this comment.
Suggested change
| import { GriffelStyle, makeResetStyles, makeStyles, mergeClasses } from '@griffel/react'; | |
| import { type GriffelStyle, makeResetStyles, makeStyles, mergeClasses } from '@griffel/react'; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Previous Behavior
Checkboxes didn't have any transition between states.
New Behavior
Checkboxes now have transition between all states, according to the specs
Related Issue(s)