This repository was archived by the owner on Mar 4, 2020. It is now read-only.
Checkbox spec: clarifying conversion table & adding work items to track conversion. #2212
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,38 +116,11 @@ https://developer.microsoft.com/en-us/fabric#/controls/web/checkbox | |
| | theme | ITheme | | | ||
| | toggle | boolean | default false | | ||
| | variables | any | | | ||
|
|
||
|
|
||
| ### Conversion process from Fabric 7 to Fluent UI Checkbox | ||
|
|
||
| #### 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? | | ||
| | -----------------------------| -------------------- | :--------------------: | :--------------: | :-------------------: | | ||
|
|
@@ -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; icon | ❌ | ❌ | ❌ | | ||
| | `className` | Yes - native | ❌ | ❌ | ❌ | | ||
| | `defaultChecked` | Yes - native | ❌ | ❌ | ❌ | | ||
| | `defaultIndetermiante` | Yes - native | ❌ | ❌ | ❌ | | ||
|
|
@@ -173,7 +147,25 @@ 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. 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. | ||
aneeshack4 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - [ ] Remove `accessibility` prop. | ||
|
There was a problem hiding this comment. as discussed on the sync last week, we should not remove accessibility prop. We can have a separate discsussion about a replacement for it There was a problem hiding this comment. Ah I wasn't aware of this - what is this separate plan/is it documented anywhere? |
||
| - [ ] 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<TextProps>`and if other type, implement that. | ||
| - [ ] Decide if, in keeping icon, it will be of type `ShorthandValue<IconProps>` and if other type, implement that. | ||
| - [ ] Change `onChange` type to native but with the added `checked` state as the 2nd prop `onChange?: (ev?: React.FormEvent<HTMLElement | HTMLInputElement>, 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. | ||
|
There was a problem hiding this comment. @levithomason was saying there were reasons to keep this that you all discussed in the second week, I thought. There was a problem hiding this comment. To keep the design prop? I don't recall or see anything about it on the hackmd notes. |
||
| - [ ] 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. | ||
|
There was a problem hiding this comment. The docs don't really mention that unrecognized native "div" (?) props should be mixed into root. I believe we expected that to continue to work. There was a problem hiding this comment. Do you mean the spec & not the docs? It's mentioned like this under the screenreader accessibility section but I'll add it here too:
|
||
| - [ ] Unrecognized native `div` props should be mixed into `root`. | ||
|
|
||
| ## Slots | ||
|
|
||
|
|
||
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.
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.
I believe all aria* attributes we weren't going to carry over the camelCased variants and instead use aria-* native attributes.
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.
The most recent thing I remembered (from before the holidays) was the opposite, using camelCased instead of native.
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.
as the acccessibility behaviors are platform independent, we opted for using aria-* initially istead of camel case. camel case is react specific and having just one approach in both components and behaviors brings consistency. @ecraig12345 it would be good to cover these decisions in design principles, so that we can refer to them and discsus them - I see pros and cons of this but the dicsussion might be lost when done under checkbox.