Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const defaultLabelRendderer = (checkboxProps?: ICheckboxProps): JSX.Element | null => {
const defaultLabelRenderer = (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> = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const V9Props: Partial<CheckboxProps> = {
const v9Props: Partial<CheckboxProps> = {

ref: componentRef,
checked: checked || indeterminate,
defaultChecked: defaultChecked || defaultIndeterminate,

labelPosition: boxSide === 'end' ? 'before' : 'after',
onChange,
} as Partial<CheckboxProps>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} as Partial<CheckboxProps>;
};


return {
...inputProps, // This inputProps is specific for the input element, and the html attributes are also used here instead of props.
'aria-describedby': ariaDescribedBy,
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Author

Choose a reason for hiding this comment

The 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),
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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([
Copy link
Member

Choose a reason for hiding this comment

The 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 {
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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,62 @@
import * as React from 'react';

import descriptionMd from './Description.md';

import { Checkbox } from '@fluentui/react';
import {
FluentProvider,
Checkbox as CheckboxV9,
webLightTheme,
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} />
<FluentProvider theme={webLightTheme}>
<CheckboxShim label="checkbox" required={true} />
</FluentProvider>
<FluentProvider theme={webLightTheme}>
<CheckboxV9 label="checkbox" required={true} />
</FluentProvider>
</div>
);
};

export default {
title: 'Migration Shims/V8/CheckboxShim',
component: CheckboxShim,
parameters: {
docs: {
description: {
component: [descriptionMd].join('\n'),
},
},
},
};