From f533233717a4814a983e87a24839995813bcedf3 Mon Sep 17 00:00:00 2001 From: Aneesha Kommineni Date: Tue, 7 Jan 2020 14:57:01 -0800 Subject: [PATCH 1/3] Checkbox spec: making conversion of Fabric to new Fluent Checkbox table more clear, adding work items to track conversion of current Fluent/Stardust to new Fluent Checkbox. --- specs/Checkbox.md | 51 +++++++++++++++++++---------------------------- 1 file changed, 21 insertions(+), 30 deletions(-) diff --git a/specs/Checkbox.md b/specs/Checkbox.md index 04af936b31..9e4505d26e 100644 --- a/specs/Checkbox.md +++ b/specs/Checkbox.md @@ -116,38 +116,11 @@ https://developer.microsoft.com/en-us/fabric#/controls/web/checkbox | theme | ITheme | | | toggle | boolean | default false | | variables | any | | - -### Recommended props - -| Name | Type | -| -------------- | ----------------------------------------------------------- | -| ariaDescribedBy | string | -| ariaLabel | string | -| ariaLabelledBy | string | -| as | keyof JSX.IntrinsicElements | -| checked | boolean | -| className | string | -| defaultChecked | boolean | -| defaultIndeterminate | boolean | -| disabled | boolean | -| indeterminate | boolean | -| label | string | -| name | string | -| onChange | (ev: Event, value: boolean) => void | -| labelPosition | start or end | -Note: rtl, styles, and theme come from compose or the ThemeProvider. And name has been added to support checkbox in form scenarios. - -Removing the following two props because the ARIA spec dictates role='checkbox' doesn't need aria-posinset and aria-setsize. These are only valid for role='option' which is only in the case the checkbox is a part of a listbox, which is not something we need to account for in the base component API. If the user does need to provide these two props, slotProps could be used to apply additional props to any slot. - -| Name | Concern | -| ------------------------------------- | ----------------------------------------------------------------- | -| ariaPositionInset | if checkbox is in a set, should be up to the user to provide a11y | -| ariaSetSize | same as above | ### Conversion process from Fabric 7 to Fluent UI Checkbox -#### Fluent Checkbox recommended props interface +#### Fluent Checkbox recommended props interface - FINAL PROPS LIST; this is to track the transition from current Fabric checkbox to the new Fluent checkbox we're building with this spec. | Name | To transition or not?| Property transitioned? | Breaking change? | Codemod/Shim created? | | -----------------------------| -------------------- | :--------------------: | :--------------: | :-------------------: | @@ -156,9 +129,10 @@ Removing the following two props because the ARIA spec dictates role='checkbox' | `ariaLabelledBy` | User provided | ❌ | ❌ | ❌ | | `ariaPositionInSet` | Won't be transitioned| ❌ | ❌ | ❌ | | `ariaSetSize` | Won't be transitioned| ❌ | ❌ | ❌ | +| `as` | TBD | ❌ | ❌ | ❌ | | `boxSide` | No; labelPosition | ❌ | ❌ | ❌ | | `checked` | Yes - native | ❌ | ❌ | ❌ | -| `checkmarkIconProps` | No | ❌ | ❌ | ❌ | +| `checkmarkIconProps` | No; icon | ❌ | ❌ | ❌ | | `className` | Yes - native | ❌ | ❌ | ❌ | | `defaultChecked` | Yes - native | ❌ | ❌ | ❌ | | `defaultIndetermiante` | Yes - native | ❌ | ❌ | ❌ | @@ -173,7 +147,24 @@ Removing the following two props because the ARIA spec dictates role='checkbox' Props being removed: -ariaPoisitionInSet and ariaSetSize - when writing parent component, user should set these on the checkbox. +ariaPoisitionInSet and ariaSetSize - when writing parent component, user should set these on the checkbox. ARIA spec dictates role='checkbox' doesn't need aria-posinset and aria-setsize. These are only valid for role='option' which is only in the case the checkbox is a part of a listbox, which is not something we need to account for in the base component API. If the user does need to provide these two props, slotProps could be used to apply additional props to any slot. + +Note: rtl, styles, and theme come from compose or the ThemeProvider. And name has been added to support checkbox in form scenarios. + +### Work items and tracking progress on conversion from the current Fluent (Stardust) checkbox to the new Fluent checkbox we're building with this spec: +- [ ] Remove `animation` prop. +- [ ] Remove `accessibility` prop. +- [ ] Add `indeterminate` and `defaultIndeterminate` props to support the third state. +- [ ] Redesign `keytipProps` from what's currently in Fabric and add the new implementation. +- [ ] Decide if, in keeping label, it will be of type `ShorthandValue`and if other type, implement that. +- [ ] Decide if, in keeping icon, it will be of type `ShorthandValue` and if other type, implement that. +- [ ] Change `onChange` type to native `onChange?: (ev?: React.FormEvent, checked?: boolean) => void;` from current `ComponentEventHandler`. +- [ ] Decide if `onClick` is too similar to `onChange` and if so, remove it. If not, change its type to native instead of the current `ComponentEventHandler`. +- [ ] Remove `toggle` - it will be a separate variant component of Checkbox. +- [ ] Remove `variables` - theming will be handled differently in the new Fluent. +- [ ] Remove `design` prop. +- [ ] Decide on the TBD props `as`, `styles`, and `theme` which will apply to other components across the new Fluent library. +- [ ] Keep `labelPosition`, `checked`, `defaultChecked`, `className`, and `disabled` props unchanged. ## Slots From 8a1bb1591feef444d2897bcb6ba695bc591386b0 Mon Sep 17 00:00:00 2001 From: Aneesha Kommineni Date: Tue, 7 Jan 2020 14:59:49 -0800 Subject: [PATCH 2/3] Clarifying further what the first conversion table is for --- specs/Checkbox.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/Checkbox.md b/specs/Checkbox.md index 9e4505d26e..6f56f95239 100644 --- a/specs/Checkbox.md +++ b/specs/Checkbox.md @@ -120,7 +120,7 @@ https://developer.microsoft.com/en-us/fabric#/controls/web/checkbox ### Conversion process from Fabric 7 to Fluent UI Checkbox -#### Fluent Checkbox recommended props interface - FINAL PROPS LIST; this is to track the transition from current Fabric checkbox to the new Fluent checkbox we're building with this spec. +#### Fluent Checkbox recommended props interface - FINAL PROPS LIST of the new Fluent checkbox. We came to this by looking at the props list of the current Fabric Checkbox so the transitioning progress is meant to track the conversion of the current Fabric Checkbox to the new Fluent Checkbox. | Name | To transition or not?| Property transitioned? | Breaking change? | Codemod/Shim created? | | -----------------------------| -------------------- | :--------------------: | :--------------: | :-------------------: | From e2213a7bc62b8ddedc0bf6f2ef3ba8e5a10b4ac0 Mon Sep 17 00:00:00 2001 From: Aneesha Kommineni Date: Fri, 10 Jan 2020 16:49:27 -0800 Subject: [PATCH 3/3] responding to feedback --- specs/Checkbox.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/specs/Checkbox.md b/specs/Checkbox.md index 6f56f95239..b53fb8d527 100644 --- a/specs/Checkbox.md +++ b/specs/Checkbox.md @@ -158,13 +158,14 @@ Note: rtl, styles, and theme come from compose or the ThemeProvider. And name ha - [ ] Redesign `keytipProps` from what's currently in Fabric and add the new implementation. - [ ] Decide if, in keeping label, it will be of type `ShorthandValue`and if other type, implement that. - [ ] Decide if, in keeping icon, it will be of type `ShorthandValue` and if other type, implement that. -- [ ] Change `onChange` type to native `onChange?: (ev?: React.FormEvent, checked?: boolean) => void;` from current `ComponentEventHandler`. -- [ ] Decide if `onClick` is too similar to `onChange` and if so, remove it. If not, change its type to native instead of the current `ComponentEventHandler`. +- [ ] Change `onChange` type to native but with the added `checked` state as the 2nd prop `onChange?: (ev?: React.FormEvent, checked?: boolean) => void;` from current `ComponentEventHandler`. +- [ ] Remove `onClick` because it too similar to `onChange`. - [ ] Remove `toggle` - it will be a separate variant component of Checkbox. - [ ] Remove `variables` - theming will be handled differently in the new Fluent. - [ ] Remove `design` prop. - [ ] Decide on the TBD props `as`, `styles`, and `theme` which will apply to other components across the new Fluent library. - [ ] Keep `labelPosition`, `checked`, `defaultChecked`, `className`, and `disabled` props unchanged. +- [ ] Unrecognized native `div` props should be mixed into `root`. ## Slots