(react-slider) - Adding the initial Spec.md for the converged Slider component #19051

czearing · 2021-07-21T17:18:46Z

Pull request checklist

Addresses an existing issue: Slider Convergence #18886

Description of changes

Implementing the initial Spec.md file for the converged slider component.

…ix/react-slider-api-spec-update

fabricteam · 2021-07-21T17:21:47Z

📊 Bundle size report

_{🤖 This report was generated against 499afa391b58984c3d509bdf5d9b7e6428f833a2}

codesandbox-ci · 2021-07-21T17:26:00Z

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 fa71d25:

Sandbox	Source
Fluent UI React Starter	Configuration

size-auditor · 2021-07-21T17:29:26Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 499afa391b58984c3d509bdf5d9b7e6428f833a2 (build)

fabricteam · 2021-07-21T17:46:21Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	953	934	5000
BaseButton	mount	974	1011	5000
Breadcrumb	mount	2682	2652	1000
ButtonNext	mount	470	435	5000
Checkbox	mount	1683	1677	5000
CheckboxBase	mount	1466	1430	5000
ChoiceGroup	mount	5082	5236	5000
ComboBox	mount	1006	1055	1000
CommandBar	mount	10462	10625	1000
ContextualMenu	mount	6781	6689	1000
DefaultButton	mount	1290	1242	5000
DetailsRow	mount	4141	4287	5000
DetailsRowFast	mount	4149	3927	5000
DetailsRowNoStyles	mount	3853	3853	5000
Dialog	mount	2198	2167	1000
DocumentCardTitle	mount	147	157	1000
Dropdown	mount	3480	3317	5000
FluentProviderNext	mount	7848	7510	5000
FocusTrapZone	mount	1937	1961	5000
FocusZone	mount	1980	2044	5000
IconButton	mount	1900	2009	5000
Label	mount	374	375	5000
Layer	mount	2018	1994	5000
Link	mount	485	527	5000
MakeStyles	mount	1839	1793	50000
MenuButton	mount	1561	1631	5000
MessageBar	mount	2084	2153	5000
Nav	mount	3595	3537	1000
OverflowSet	mount	1159	1156	5000
Panel	mount	2301	2209	1000
Persona	mount	932	938	1000
Pivot	mount	1569	1598	1000
PrimaryButton	mount	1428	1448	5000
Rating	mount	8726	8601	5000
SearchBox	mount	1480	1518	5000
Shimmer	mount	2716	2722	5000
Slider	mount	2156	2141	5000
SpinButton	mount	5412	5452	5000
Spinner	mount	460	452	5000
SplitButton	mount	3293	3521	5000
Stack	mount	541	516	5000
StackWithIntrinsicChildren	mount	1745	1850	5000
StackWithTextChildren	mount	5124	5167	5000
SwatchColorPicker	mount	11059	11541	5000
Tabs	mount	1507	1552	1000
TagPicker	mount	2858	2865	5000
TeachingBubble	mount	12372	12729	5000
Text	mount	458	457	5000
TextField	mount	1566	1506	5000
ThemeProvider	mount	1294	1341	5000
ThemeProvider	virtual-rerender	626	632	5000
Toggle	mount	867	849	5000
buttonNative	mount	124	116	5000

Perf Analysis (`@fluentui/react-northstar`)

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
TreeWith60ListItems.default	192	173	1.11:1
DividerMinimalPerf.default	403	373	1.08:1
StatusMinimalPerf.default	802	746	1.08:1
IconMinimalPerf.default	673	621	1.08:1
LayoutMinimalPerf.default	410	388	1.06:1
CardMinimalPerf.default	626	594	1.05:1
ChatWithPopoverPerf.default	408	390	1.05:1
HeaderMinimalPerf.default	395	375	1.05:1
RadioGroupMinimalPerf.default	494	469	1.05:1
ReactionMinimalPerf.default	407	389	1.05:1
RefMinimalPerf.default	226	216	1.05:1
BoxMinimalPerf.default	390	374	1.04:1
ButtonMinimalPerf.default	185	178	1.04:1
ButtonSlotsPerf.default	610	586	1.04:1
DropdownManyItemsPerf.default	753	727	1.04:1
FlexMinimalPerf.default	299	288	1.04:1
GridMinimalPerf.default	361	347	1.04:1
ListCommonPerf.default	722	695	1.04:1
MenuMinimalPerf.default	910	879	1.04:1
PortalMinimalPerf.default	174	168	1.04:1
AnimationMinimalPerf.default	459	445	1.03:1
DialogMinimalPerf.default	802	779	1.03:1
LoaderMinimalPerf.default	723	703	1.03:1
PopupMinimalPerf.default	599	584	1.03:1
ProviderMergeThemesPerf.default	1669	1624	1.03:1
TableMinimalPerf.default	442	430	1.03:1
CustomToolbarPrototype.default	4001	3889	1.03:1
ToolbarMinimalPerf.default	979	949	1.03:1
TreeMinimalPerf.default	840	815	1.03:1
AttachmentMinimalPerf.default	173	170	1.02:1
ButtonOverridesMissPerf.default	1871	1839	1.02:1
CarouselMinimalPerf.default	509	498	1.02:1
ChatDuplicateMessagesPerf.default	323	317	1.02:1
DatepickerMinimalPerf.default	5850	5762	1.02:1
InputMinimalPerf.default	1312	1287	1.02:1
ItemLayoutMinimalPerf.default	1306	1285	1.02:1
LabelMinimalPerf.default	467	459	1.02:1
SegmentMinimalPerf.default	370	361	1.02:1
SliderMinimalPerf.default	1621	1588	1.02:1
TextMinimalPerf.default	384	377	1.02:1
CheckboxMinimalPerf.default	2925	2909	1.01:1
FormMinimalPerf.default	450	447	1.01:1
HeaderSlotsPerf.default	830	820	1.01:1
MenuButtonMinimalPerf.default	1729	1705	1.01:1
TableManyItemsPerf.default	2044	2024	1.01:1
AlertMinimalPerf.default	302	301	1:1
AvatarMinimalPerf.default	213	212	1:1
EmbedMinimalPerf.default	4282	4279	1:1
ListNestedPerf.default	600	602	1:1
ListMinimalPerf.default	565	568	0.99:1
TextAreaMinimalPerf.default	559	562	0.99:1
AttachmentSlotsPerf.default	1152	1171	0.98:1
DropdownMinimalPerf.default	3084	3154	0.98:1
ImageMinimalPerf.default	405	413	0.98:1
ChatMinimalPerf.default	708	730	0.97:1
ProviderMinimalPerf.default	1031	1066	0.97:1
SplitButtonMinimalPerf.default	4004	4137	0.97:1
TooltipMinimalPerf.default	1036	1078	0.96:1
AccordionMinimalPerf.default	161	169	0.95:1
RosterPerf.default	1258	1320	0.95:1
SkeletonMinimalPerf.default	360	377	0.95:1
ListWith60ListItems.default	666	714	0.93:1
VideoMinimalPerf.default	641	692	0.93:1

packages/react-slider/Spec.md

Co-authored-by: Esteban Munoz <[email protected]>

…ix/react-slider-api-spec-update

…/czearing/fluentui into fix/react-slider-api-spec-update # Conflicts: # packages/react-slider/Spec.md

packages/react-slider/Spec.md

khmakoto · 2021-07-23T19:18:25Z

packages/react-slider/Spec.md

+```jsx
+    <div className="ms-Slider-root">
+        // Label is up for discussion
+        <div className="ms-Slider-label">
+            {...label children}
+        </div>
+        <div className="ms-Slider-container">
+            <div className="ms-Slider-rail" />
+            <div className="ms-Slider-markContainer">
+                <div className="ms-Slider-mark" />
+                <div className="ms-Slider-markLabel" />
+            </div>
+            <div className="ms-Slider-track" />
+            <div className="ms-Slider-thumb" />
+        </div>
+        <div className="ms-Slider-valueLabel" />
+    </div>
+```


I would specially like to see what @smhigley thinks of the structure proposed here since I know that we might not want everything to be a div due to accessibility purposes (maybe we need to render a non-visible input).

packages/react-slider/Spec.md

khmakoto · 2021-07-23T19:20:02Z

packages/react-slider/Spec.md

+### Removing Label and ValueLabel
+
+If there is a way to remove the Label and ValueLabel while preserving accessibility it could greatly benefit the render structure (Removes container, label, and valueLabel slots).


Again, especially interested of what @smhigley thinks of here.

I'm not sure what the purpose of valueLabel is, specific to accessibility. I thought it was a visual thing, e.g. if someone wanted text showing the current value to be present, and update as you changed the value?

Label IMO should stay internal, but that seems like a separate discussion.

Co-authored-by: Makoto Morimoto <[email protected]>

…ix/react-slider-api-spec-update

…riptions of Slider using the RangedSlider API.

…ix/react-slider-api-spec-update

smhigley · 2021-07-28T17:18:28Z

packages/react-slider/Spec.md

+| Name              | <img src="https://img.shields.io/badge/Used%20in-v0-orange" alt="drawing" width="200"/> | <img src="https://img.shields.io/badge/Used%20in-v8-blue" alt="drawing" width="200"/> | Description                                                                                                   |
+| ----------------- | --------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- |
+| label             | x                                                                                       | &check;                                                                               | The description label of the **Slider**.                                                                      |
+| valueLabel        | x                                                                                       | &check;                                                                               | The current value or unique format to be shown for the **Slider's** `value label`.                            |


I'm wondering if this should be a function that takes the value and returns a string, since if someone wants custom value text, they probably also want it for the min/max values.

If that's already your plan, then ignore this 😄

What I was thinking for the valueLabel prop is that it would handle the two previous API features: valueFormat and showValue.
If someone wanted to render a valueLabel they could:

Pass the Boolean true and receive the currentValue.

Pass a method like this to customize it:

const sliderValue = (value: number) => `${value}%`;

Got it, thanks for the explanation! I still wonder if it might be better to split those out though. For example, what if someone wanted to customize the min/max labels + maybe the tick labels, but not show separate text for the current value?

smhigley · 2021-07-28T17:20:11Z

packages/react-slider/Spec.md

+| vertical          | &check;                                                                                 | &check;                                                                               | Whether to render the **Slider** vertically. @default `false` (render horizontally)                           |
+| marks             | x                                                                                       | x                                                                                     | Whether the **Slider** will have marks to visibly display its steps. @default `false` (renders without marks) |
+| fluid             | x                                                                                       | &check;                                                                               | A **Slider** can take the width of its container. @default `false` (width does not fill the container)        |
+| disabledFocusable | X                                                                                       | X                                                                                     | A **Slider** can be disabled and focusable at the same time.                                                  |


This seems weird to me -- disabledFocusable makes sense for a button or menuitem, but why is this an option for a Slider?

The accessibility design mentioned that the disabled Slider should allow focus but be readonly to assist screen readers in determining if an element was disabled. The disabledFocusable event could be added to follow similar patterns in other converged components. Out of curiosity, how would you suggest handling the disabled behavior of the Slider? Should the disabled Slider allow focus in the first place?

packages/react-slider/Spec.md

smhigley · 2021-07-28T17:32:41Z

packages/react-slider/Spec.md

- Identify UI parts that appear on **hover or focus** and specify keyboard and screen reader interaction with them
- List cases when **focus** needs to be **trapped** in sections of the UI (for dialogs and popups or for hierarchical navigation)
- List cases when **focus** needs to be **moved programatically** (if parts of the UI are appearing/disappearing or other cases)
+`Slider` design pattern: [W3 Slider](https://www.w3.org/TR/wai-aria-1.1/#slider)


The APG actually isn't a good reference for how to build a slider. The purpose of the APG is to show how to use ARIA, but not whether it should be used. In cases where an actual HTML element exists (and especially in this one), it's much, much better to use the HTML element.

Here's a (very old, sorry 😅) demo of creating a custom track, thumb, and fill on top of an <input type="range">: https://codepen.io/smhigley/pen/ObWbdy

Thanks for sharing the codepen!

smhigley · 2021-07-28T17:36:32Z

packages/react-slider/Spec.md

+### Removing Label and ValueLabel
+
+If there is a way to remove the Label and ValueLabel while preserving accessibility it could greatly benefit the render structure (Removes container, label, and valueLabel slots).


I'm not sure what the purpose of valueLabel is, specific to accessibility. I thought it was a visual thing, e.g. if someone wanted text showing the current value to be present, and update as you changed the value?

Label IMO should stay internal, but that seems like a separate discussion.

…ix/react-slider-api-spec-update

…ix/react-slider-api-spec-update # Conflicts: # packages/react-slider/package.json

1. Removing valueLabel, Fluid, and snapToStep 2. Updating render structure 3. Updating Topics for Discussion

packages/react-slider/Spec.md

czearing added 5 commits July 13, 2021 16:32

Adding API section to Spec.md

78153af

Adding spec

07b5652

Merge branch 'master' of https://github.com/microsoft/fluentui into f…

abb1b73

…ix/react-slider-api-spec-update

Typo fix

4169a6d

Removing unnecessary markdown

8201876

czearing added Component: Slider Type: Spec Component spec PR labels Jul 21, 2021

czearing requested review from dzearing, levithomason, miroslavstastny and behowell July 21, 2021 17:18

czearing requested a review from a team as a code owner July 21, 2021 17:18

msft-fluent-ui-bot assigned ling1726 Jul 21, 2021

sopranopillow reviewed Jul 21, 2021

View reviewed changes

packages/react-slider/Spec.md Outdated Show resolved Hide resolved

czearing and others added 6 commits July 21, 2021 12:39

Update packages/react-slider/Spec.md

790f67a

Co-authored-by: Esteban Munoz <[email protected]>

Merge branch 'master' of https://github.com/microsoft/fluentui into f…

9e5c1b2

…ix/react-slider-api-spec-update

Updating mark location to be behind track and thumb.

61d33d3

Merge branch 'fix/react-slider-api-spec-update' of https://github.com…

0f4e938

…/czearing/fluentui into fix/react-slider-api-spec-update # Conflicts: # packages/react-slider/Spec.md

Quick fix to the public section of the spec.

01d2127

Updating package dep in react-slider.

0a7fb7d

khmakoto reviewed Jul 23, 2021

View reviewed changes

czearing and others added 5 commits July 23, 2021 12:38

Update packages/react-slider/Spec.md

5babc7c

Co-authored-by: Makoto Morimoto <[email protected]>

Update packages/react-slider/Spec.md

0a232df

Co-authored-by: Makoto Morimoto <[email protected]>

Update packages/react-slider/Spec.md

e9533d2

Co-authored-by: Makoto Morimoto <[email protected]>

Update packages/react-slider/Spec.md

5e10fd4

Co-authored-by: Makoto Morimoto <[email protected]>

Merge branch 'master' of https://github.com/microsoft/fluentui into f…

5bc9b5e

…ix/react-slider-api-spec-update

czearing added 3 commits July 23, 2021 13:13

Proposing RangedSlider under migration and removing examples and desc…

aefec69

…riptions of Slider using the RangedSlider API.

Merge branch 'master' of https://github.com/microsoft/fluentui into f…

52d6246

…ix/react-slider-api-spec-update

Updating package and introducing activeRail element.

58ef811

khmakoto approved these changes Jul 27, 2021

View reviewed changes

smhigley reviewed Jul 28, 2021

View reviewed changes

czearing added 2 commits July 28, 2021 11:52

Merge branch 'master' of https://github.com/microsoft/fluentui into f…

9cc2ca1

…ix/react-slider-api-spec-update

Removing ariaValueLabel prop.

a5e32c0

czearing mentioned this pull request Jul 29, 2021

(react-slider) - Implementing basic Slider component #19187

Merged

2 tasks

czearing added 2 commits August 5, 2021 18:57

Merge branch 'master' of https://github.com/microsoft/fluentui into f…

97b9ad6

…ix/react-slider-api-spec-update # Conflicts: # packages/react-slider/package.json

Updating the spec:

1980968

1. Removing valueLabel, Fluid, and snapToStep 2. Updating render structure 3. Updating Topics for Discussion

khmakoto reviewed Aug 6, 2021

View reviewed changes

packages/react-slider/Spec.md Outdated Show resolved Hide resolved

PR feedback

fa71d25

czearing merged commit d92224e into microsoft:master Aug 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(react-slider) - Adding the initial Spec.md for the converged Slider component #19051

(react-slider) - Adding the initial Spec.md for the converged Slider component #19051

czearing commented Jul 21, 2021

fabricteam commented Jul 21, 2021 •

edited

Loading

codesandbox-ci bot commented Jul 21, 2021 •

edited

Loading

size-auditor bot commented Jul 21, 2021 •

edited

Loading

fabricteam commented Jul 21, 2021 •

edited

Loading

khmakoto Jul 23, 2021

khmakoto Jul 23, 2021

smhigley Jul 28, 2021

smhigley Jul 28, 2021

czearing Jul 28, 2021

smhigley Jul 28, 2021

smhigley Jul 28, 2021

czearing Jul 28, 2021 •

edited

Loading

smhigley Jul 28, 2021

czearing Jul 28, 2021

smhigley Jul 28, 2021

		### Removing Label and ValueLabel

		If there is a way to remove the Label and ValueLabel while preserving accessibility it could greatly benefit the render structure (Removes container, label, and valueLabel slots).

(react-slider) - Adding the initial Spec.md for the converged Slider component #19051

(react-slider) - Adding the initial Spec.md for the converged Slider component #19051

Conversation

czearing commented Jul 21, 2021

Pull request checklist

Description of changes

fabricteam commented Jul 21, 2021 • edited Loading

📊 Bundle size report

codesandbox-ci bot commented Jul 21, 2021 • edited Loading

size-auditor bot commented Jul 21, 2021 • edited Loading

Asset size changes

fabricteam commented Jul 21, 2021 • edited Loading

Perf Analysis (@fluentui/react-northstar)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czearing Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabricteam commented Jul 21, 2021 •

edited

Loading

codesandbox-ci bot commented Jul 21, 2021 •

edited

Loading

size-auditor bot commented Jul 21, 2021 •

edited

Loading

fabricteam commented Jul 21, 2021 •

edited

Loading

Perf Analysis (`@fluentui/react-northstar`)

czearing Jul 28, 2021 •

edited

Loading