-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add V9 Shim for V8 Checkbox #29620
base: master
Are you sure you want to change the base?
Add V9 Shim for V8 Checkbox #29620
Changes from all commits
92906f5
3b80c97
991d5b2
31c5db2
b4e9202
4eeddcc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"type": "minor", | ||
"comment": "Add v9 shim for v8 checkbox", | ||
"packageName": "@fluentui/react-migration-v8-v9", | ||
"email": "[email protected]", | ||
"dependentChangeType": "patch" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
import * as React from 'react'; | ||
import { classNamesFunction, ICheckboxProps, ICheckboxStyles, ICheckboxStyleProps } from '@fluentui/react'; | ||
import { Checkbox, makeStyles, mergeClasses } from '@fluentui/react-components'; | ||
import { useControllableValue } from '@fluentui/react-hooks'; | ||
|
||
import { shimCheckboxProps } from './shimCheckboxProps'; | ||
|
||
const useCheckboxStyles = makeStyles({ | ||
root: { | ||
display: 'flex', | ||
}, | ||
}); | ||
|
||
const getClassNames = classNamesFunction<ICheckboxStyleProps, ICheckboxStyles>({ | ||
useStaticStyles: true, | ||
}); | ||
|
||
export const CheckboxShim = (props: ICheckboxProps) => { | ||
const { className, styles: stylesV8, onRenderLabel, label } = props; | ||
const shimProps = shimCheckboxProps(props); | ||
const styles = getClassNames(stylesV8); | ||
const stylesV9 = useCheckboxStyles(); | ||
const [isChecked, setIsChecked] = useControllableValue(props.checked, props.defaultChecked, props.onChange); | ||
|
||
const onChange = React.useCallback( | ||
(event: React.ChangeEvent<HTMLElement>): void => { | ||
setIsChecked(!isChecked, event); | ||
}, | ||
[setIsChecked, isChecked], | ||
); | ||
|
||
shimProps.checked = isChecked; | ||
shimProps.onChange = onChange; | ||
|
||
const defaultLabelRendderer = (checkboxProps?: ICheckboxProps): JSX.Element | null => { | ||
if (!checkboxProps) { | ||
return null; | ||
} | ||
const { label, title } = checkboxProps; | ||
return label ? ( | ||
<span title={title} className={styles.text}> | ||
{label} | ||
</span> | ||
) : null; | ||
}; | ||
|
||
if (label || onRenderLabel) { | ||
shimProps.label = { | ||
className: mergeClasses('ms-Checkbox-text', styles.label, styles.text), | ||
children: onRenderLabel ? onRenderLabel(props, defaultLabelRendderer) : label, | ||
}; | ||
} | ||
|
||
return ( | ||
<Checkbox | ||
{...shimProps} | ||
className={mergeClasses(stylesV9.root, 'ms-Checkbox', className, styles.root)} | ||
indicator={{ className: mergeClasses('ms-Checkbox-checkbox', styles.checkbox) }} | ||
/> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export * from './CheckboxShim'; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,86 @@ | ||||||
import { ICheckboxProps } from '@fluentui/react'; | ||||||
import { CheckboxOnChangeData, CheckboxProps } from '@fluentui/react-components'; | ||||||
|
||||||
import { getHTMLAttributes } from '../utils'; | ||||||
|
||||||
// https://react.fluentui.dev/?path=/docs/concepts-migration-from-v8-components-checkbox-migration--page | ||||||
// https://github.com/microsoft/fluentui/blob/master/packages/react/src/components/Checkbox/Checkbox.types.ts | ||||||
// https://github.com/microsoft/fluentui/blob/103b8977f8d5f8dd8c430bab46ff5308a2c76371/packages/react-components/react-checkbox/src/components/Checkbox/Checkbox.types.ts | ||||||
|
||||||
export const shimCheckboxProps = (props: ICheckboxProps): CheckboxProps => { | ||||||
const { | ||||||
ariaDescribedBy, | ||||||
ariaLabel, | ||||||
ariaLabelledBy, | ||||||
ariaPositionInSet, | ||||||
ariaSetSize, | ||||||
boxSide, | ||||||
checked, | ||||||
componentRef, | ||||||
defaultChecked, | ||||||
defaultIndeterminate, | ||||||
disabled, | ||||||
indeterminate, | ||||||
inputProps, | ||||||
name, | ||||||
onChange: onChangeV8, | ||||||
required, | ||||||
title, | ||||||
} = props; | ||||||
|
||||||
const onChange = | ||||||
onChangeV8 && | ||||||
((ev: React.ChangeEvent<HTMLInputElement>, data: CheckboxOnChangeData) => { | ||||||
return onChangeV8(ev, data.checked as boolean); | ||||||
}); | ||||||
|
||||||
const V9Props: Partial<CheckboxProps> = { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
ref: componentRef, | ||||||
checked: checked || indeterminate, | ||||||
defaultChecked: defaultChecked || defaultIndeterminate, | ||||||
|
||||||
labelPosition: boxSide === 'end' ? 'before' : 'after', | ||||||
onChange, | ||||||
} as Partial<CheckboxProps>; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
return { | ||||||
...inputProps, // This inputProps is specific for the input element, and the html attributes are also used here instead of props. | ||||||
'aria-describedby': ariaDescribedBy, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Is it duplicate to write aria-describedby here when there is the getHTMLAttributes that includes it too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should not be duplicated, as the "aria-describedby" is defined as ariaDescribedBy in V8, need to be translated to the origin html. |
||||||
'aria-label': ariaLabel, | ||||||
'aria-labelledby': ariaLabelledBy, | ||||||
'aria-posinset': ariaPositionInSet, | ||||||
'aria-setsize': ariaSetSize, | ||||||
disabled, | ||||||
required, | ||||||
title, | ||||||
name, | ||||||
...V9Props, | ||||||
...getHTMLAttributes(props, CHECKBOX_PROPS_V8), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we want to spread all the v8 props on to the v9 component as the v9 component won't do anything with them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's mainly to get all the missed htmlattributes, and we will also delete some v8 props in getHTMLAttributes |
||||||
} as CheckboxProps; | ||||||
}; | ||||||
|
||||||
const CHECKBOX_PROPS_V8: Set<string> = new Set([ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Avoid using things before declaring them. |
||||||
'ariaDescribedBy', | ||||||
'ariaLabel', | ||||||
'ariaLabelledBy', | ||||||
'ariaPositionInSet', | ||||||
'ariaSetSize', | ||||||
'boxSide', | ||||||
'checked', | ||||||
'checkmarkIconProps', // one case used this | ||||||
'className', | ||||||
'componentRef', | ||||||
'defaultChecked', | ||||||
'defaultIndeterminate', | ||||||
'disabled', | ||||||
'indeterminate', | ||||||
'inputProps', | ||||||
'label', | ||||||
'name', | ||||||
'onChange', | ||||||
'onRenderLabel', | ||||||
'required', | ||||||
'styles', | ||||||
'theme', | ||||||
'title', | ||||||
]); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
export function getHTMLAttributes<T extends object>(props: T, allV8PropsSet: Set<string>): T { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider putting this in shimCheckboxProps.ts as it is only called from there. If it is useful to other shims we could move it higher in the source tree later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's a common function shared in other shims. |
||
const v8Props: T = { ...props }; | ||
const propsKeys: string[] = Object.keys(v8Props); | ||
propsKeys.forEach(key => { | ||
if (allV8PropsSet.has(key)) { | ||
delete v8Props[key as keyof typeof v8Props]; | ||
} | ||
}); | ||
return v8Props; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
The CheckboxShim component accepts the same props as a v8 Checkbox and renders a v9 Checkbox. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
import * as React from 'react'; | ||
|
||
import descriptionMd from './Description.md'; | ||
|
||
import { Checkbox } from '@fluentui/react'; | ||
import { Checkbox as CheckboxV9, makeStyles, shorthands } from '@fluentui/react-components'; | ||
import { CheckboxShim } from '../../components/Checkbox/index'; | ||
|
||
const useStyles = makeStyles({ | ||
root: { | ||
display: 'grid', | ||
gridTemplateColumns: 'auto auto auto', | ||
gridTemplateRows: '1fr', | ||
width: 'fit-content', | ||
alignContent: 'center', | ||
alignItems: 'center', | ||
justifyItems: 'center', | ||
columnGap: '10px', | ||
rowGap: '10px', | ||
}, | ||
componentName: { | ||
justifySelf: 'end', | ||
...shorthands.margin(0, '10px', 0, 0), | ||
}, | ||
}); | ||
|
||
export const Default = () => { | ||
const styles = useStyles(); | ||
|
||
return ( | ||
<div className={styles.root}> | ||
<h3>v8</h3> | ||
<h3>shim</h3> | ||
<h3>v9</h3> | ||
<Checkbox label="checkbox" required={true} /> | ||
<CheckboxShim label="checkbox" required={true} /> | ||
<CheckboxV9 label="checkbox" required={true} /> | ||
</div> | ||
); | ||
}; | ||
|
||
export default { | ||
title: 'Migration Shims/V8/CheckboxShim', | ||
component: CheckboxShim, | ||
parameters: { | ||
docs: { | ||
description: { | ||
component: [descriptionMd].join('\n'), | ||
}, | ||
}, | ||
}, | ||
}; |
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.