Add V9 Shim for V8 Checkbox #29620
Add V9 Shim for V8 Checkbox #29620
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4eeddcc:
|
packages/react-components/react-migration-v8-v9/src/stories/CheckboxShim/index.stories.tsx
Outdated
Show resolved
Hide resolved
| shimProps.checked = isChecked; | ||
| shimProps.onChange = onChange; | ||
|
|
||
| const defaultLabelRendderer = (checkboxProps?: ICheckboxProps): JSX.Element | null => { |
There was a problem hiding this comment.
| const defaultLabelRendderer = (checkboxProps?: ICheckboxProps): JSX.Element | null => { | |
| const defaultLabelRenderer = (checkboxProps?: ICheckboxProps): JSX.Element | null => { |
| return onChangeV8(ev, data.checked as boolean); | ||
| }); | ||
|
|
||
| const V9Props: Partial<CheckboxProps> = { |
There was a problem hiding this comment.
| const V9Props: Partial<CheckboxProps> = { | |
| const v9Props: Partial<CheckboxProps> = { |
|
|
||
| labelPosition: boxSide === 'end' ? 'before' : 'after', | ||
| onChange, | ||
| } as Partial<CheckboxProps>; |
There was a problem hiding this comment.
| } as Partial<CheckboxProps>; | |
| }; |
| @@ -0,0 +1,10 @@ | |||
| export function getHTMLAttributes<T extends object>(props: T, allV8PropsSet: Set<string>): T { | |||
There was a problem hiding this comment.
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.
Yes, it's a common function shared in other shims.
| } as CheckboxProps; | ||
| }; | ||
|
|
||
| const CHECKBOX_PROPS_V8: Set<string> = new Set([ |
There was a problem hiding this comment.
Nit: Avoid using things before declaring them.
|
|
||
| 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.
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.
It should not be duplicated, as the "aria-describedby" is defined as ariaDescribedBy in V8, need to be translated to the origin html.
| title, | ||
| name, | ||
| ...V9Props, | ||
| ...getHTMLAttributes(props, CHECKBOX_PROPS_V8), |
There was a problem hiding this comment.
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.
Yes, it's mainly to get all the missed htmlattributes, and we will also delete some v8 props in getHTMLAttributes
Previous Behavior
New Behavior
Related Issue(s)