-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
(react-slider) - Adding Custom Marks and Labels #19397
Conversation
…eat/slider-mark # Conflicts: # packages/react-slider/src/Slider.stories.tsx # packages/react-slider/src/components/Slider/Slider.types.ts # packages/react-slider/src/components/Slider/useSliderState.tsx # packages/react-slider/src/components/Slider/useSliderStyles.ts
…eat/slider-mark # Conflicts: # packages/react-slider/src/components/Slider/Slider.types.ts
📊 Bundle size report🤖 This report was generated against 0198c05dad393704afbe8aeab6c5cdf6005bcc5a |
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 e7145aa:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 0198c05dad393704afbe8aeab6c5cdf6005bcc5a (build) |
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Panel | mount | 2175 | 1449 | 1000 | Possible regression |
All results
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 958 | 1013 | 5000 | |
BaseButton | mount | 1010 | 970 | 5000 | |
Breadcrumb | mount | 2749 | 2676 | 1000 | |
ButtonNext | mount | 438 | 464 | 5000 | |
Checkbox | mount | 1588 | 1655 | 5000 | |
CheckboxBase | mount | 1406 | 1398 | 5000 | |
ChoiceGroup | mount | 5157 | 5263 | 5000 | |
ComboBox | mount | 1164 | 1124 | 1000 | |
CommandBar | mount | 10761 | 10720 | 1000 | |
ContextualMenu | mount | 6391 | 6465 | 1000 | |
DefaultButton | mount | 1189 | 1210 | 5000 | |
DetailsRow | mount | 4078 | 4105 | 5000 | |
DetailsRowFast | mount | 4002 | 4019 | 5000 | |
DetailsRowNoStyles | mount | 3836 | 4078 | 5000 | |
Dialog | mount | 2274 | 2387 | 1000 | |
DocumentCardTitle | mount | 148 | 160 | 1000 | |
Dropdown | mount | 3568 | 3458 | 5000 | |
FluentProviderNext | mount | 7689 | 7328 | 5000 | |
FocusTrapZone | mount | 1810 | 1971 | 5000 | |
FocusZone | mount | 1807 | 1825 | 5000 | |
IconButton | mount | 1908 | 1872 | 5000 | |
Label | mount | 346 | 347 | 5000 | |
Layer | mount | 1940 | 1912 | 5000 | |
Link | mount | 508 | 502 | 5000 | |
MakeStyles | mount | 1914 | 1876 | 50000 | |
MenuButton | mount | 1561 | 1623 | 5000 | |
MessageBar | mount | 2277 | 2034 | 5000 | |
Nav | mount | 3471 | 3440 | 1000 | |
OverflowSet | mount | 1167 | 1146 | 5000 | |
Panel | mount | 2175 | 1449 | 1000 | Possible regression |
Persona | mount | 908 | 909 | 1000 | |
Pivot | mount | 1549 | 1536 | 1000 | |
PrimaryButton | mount | 1388 | 1432 | 5000 | |
Rating | mount | 8381 | 8425 | 5000 | |
SearchBox | mount | 1462 | 1446 | 5000 | |
Shimmer | mount | 2734 | 2755 | 5000 | |
Slider | mount | 2100 | 2080 | 5000 | |
SpinButton | mount | 5299 | 5320 | 5000 | |
Spinner | mount | 443 | 417 | 5000 | |
SplitButton | mount | 3416 | 3525 | 5000 | |
Stack | mount | 554 | 568 | 5000 | |
StackWithIntrinsicChildren | mount | 1728 | 1768 | 5000 | |
StackWithTextChildren | mount | 5116 | 5080 | 5000 | |
SwatchColorPicker | mount | 10978 | 10793 | 5000 | |
Tabs | mount | 1494 | 1700 | 1000 | |
TagPicker | mount | 2882 | 2744 | 5000 | |
TeachingBubble | mount | 12524 | 12208 | 5000 | |
Text | mount | 452 | 455 | 5000 | |
TextField | mount | 1497 | 1465 | 5000 | |
ThemeProvider | mount | 1203 | 1198 | 5000 | |
ThemeProvider | virtual-rerender | 614 | 605 | 5000 | |
Toggle | mount | 849 | 857 | 5000 | |
buttonNative | mount | 107 | 109 | 5000 |
Perf Analysis (@fluentui/react-northstar
)
Perf tests with no regressions
Scenario | Current PR Ticks | Baseline Ticks | Ratio |
---|---|---|---|
TreeMinimalPerf.default | 940 | 855 | 1.1:1 |
BoxMinimalPerf.default | 405 | 371 | 1.09:1 |
DropdownManyItemsPerf.default | 862 | 793 | 1.09:1 |
ButtonMinimalPerf.default | 189 | 175 | 1.08:1 |
CardMinimalPerf.default | 653 | 606 | 1.08:1 |
TableMinimalPerf.default | 462 | 429 | 1.08:1 |
AttachmentMinimalPerf.default | 175 | 163 | 1.07:1 |
ChatWithPopoverPerf.default | 422 | 393 | 1.07:1 |
DropdownMinimalPerf.default | 3511 | 3284 | 1.07:1 |
ListMinimalPerf.default | 570 | 540 | 1.06:1 |
RefMinimalPerf.default | 248 | 233 | 1.06:1 |
AvatarMinimalPerf.default | 216 | 205 | 1.05:1 |
ButtonSlotsPerf.default | 620 | 588 | 1.05:1 |
DatepickerMinimalPerf.default | 6080 | 5852 | 1.04:1 |
InputMinimalPerf.default | 1377 | 1323 | 1.04:1 |
LabelMinimalPerf.default | 433 | 415 | 1.04:1 |
PortalMinimalPerf.default | 192 | 185 | 1.04:1 |
VideoMinimalPerf.default | 699 | 671 | 1.04:1 |
AnimationMinimalPerf.default | 449 | 435 | 1.03:1 |
ChatDuplicateMessagesPerf.default | 313 | 304 | 1.03:1 |
ChatMinimalPerf.default | 807 | 787 | 1.03:1 |
EmbedMinimalPerf.default | 4542 | 4408 | 1.03:1 |
FormMinimalPerf.default | 464 | 452 | 1.03:1 |
ListWith60ListItems.default | 708 | 689 | 1.03:1 |
RadioGroupMinimalPerf.default | 504 | 488 | 1.03:1 |
ToolbarMinimalPerf.default | 1081 | 1053 | 1.03:1 |
AccordionMinimalPerf.default | 169 | 165 | 1.02:1 |
CarouselMinimalPerf.default | 505 | 495 | 1.02:1 |
ImageMinimalPerf.default | 461 | 453 | 1.02:1 |
ItemLayoutMinimalPerf.default | 1358 | 1333 | 1.02:1 |
MenuButtonMinimalPerf.default | 1795 | 1759 | 1.02:1 |
PopupMinimalPerf.default | 633 | 620 | 1.02:1 |
SegmentMinimalPerf.default | 392 | 385 | 1.02:1 |
CustomToolbarPrototype.default | 4404 | 4302 | 1.02:1 |
DialogMinimalPerf.default | 841 | 836 | 1.01:1 |
HeaderSlotsPerf.default | 853 | 841 | 1.01:1 |
IconMinimalPerf.default | 707 | 697 | 1.01:1 |
TextAreaMinimalPerf.default | 575 | 567 | 1.01:1 |
ButtonOverridesMissPerf.default | 1843 | 1835 | 1:1 |
CheckboxMinimalPerf.default | 2979 | 2972 | 1:1 |
GridMinimalPerf.default | 372 | 373 | 1:1 |
LayoutMinimalPerf.default | 380 | 381 | 1:1 |
ProviderMinimalPerf.default | 1093 | 1095 | 1:1 |
SliderMinimalPerf.default | 1683 | 1680 | 1:1 |
StatusMinimalPerf.default | 830 | 832 | 1:1 |
TableManyItemsPerf.default | 2110 | 2119 | 1:1 |
HeaderMinimalPerf.default | 383 | 388 | 0.99:1 |
MenuMinimalPerf.default | 915 | 928 | 0.99:1 |
RosterPerf.default | 1317 | 1337 | 0.99:1 |
SkeletonMinimalPerf.default | 397 | 401 | 0.99:1 |
SplitButtonMinimalPerf.default | 4110 | 4136 | 0.99:1 |
TooltipMinimalPerf.default | 1161 | 1167 | 0.99:1 |
TreeWith60ListItems.default | 190 | 191 | 0.99:1 |
AttachmentSlotsPerf.default | 1116 | 1141 | 0.98:1 |
ListCommonPerf.default | 691 | 705 | 0.98:1 |
ListNestedPerf.default | 597 | 609 | 0.98:1 |
ProviderMergeThemesPerf.default | 1708 | 1743 | 0.98:1 |
TextMinimalPerf.default | 383 | 392 | 0.98:1 |
DividerMinimalPerf.default | 426 | 438 | 0.97:1 |
FlexMinimalPerf.default | 311 | 319 | 0.97:1 |
AlertMinimalPerf.default | 299 | 313 | 0.96:1 |
ReactionMinimalPerf.default | 399 | 421 | 0.95:1 |
…eat/slider-mark # Conflicts: # packages/react-slider/package.json
Did you ask the designers about whether they intend to support labels for the marks? (I don't see it in the design spec.) If it's not something they've explicitly asked for, I'd strongly prefer not to support it initially, in the interest of not signing up to maintain more than we need to. |
@ecraig12345 Yes, I reached out for to the designers last week but am still awaiting a response. I added the labels to the spec during research since most component libraries that include marks also have some sort of mark label feature (Material UI and Ant Design also do this). Since we are already implementing the marks adding the label just requires adding an extra div and parsing the marks object for potential content. |
Regardless of what other libraries are doing, I still think we should wait to add the label option until it's explicitly requested--among other reasons, in case the way it's designed needs a different API, which would be a breaking change (whereas if we don't implement anything now, it could easily be added to the union type as a non-breaking change in the future). We also don't have specs from the designers about size and positioning of label elements. And it's unclear if/how the mark labels ought to be handled by screen readers, so better to just avoid the issue unless we find out it's really needed later. |
That is fair. For the time being, there is a sync with the design on Wednesday regarding the Slider component. I will make sure to bring it up to clarify if it's planned or not. If it's not a priority then I can update and remove it from the PR. |
To lower the scope of the mark implementation here I simplified the pull request in #19427 which allows the marks to just accept a: boolean or number[]. |
…eat/slider-mark # Conflicts: # packages/react-slider/etc/react-slider.api.md # packages/react-slider/package.json # packages/react-slider/src/Slider.stories.tsx # packages/react-slider/src/components/Slider/Slider.test.tsx # packages/react-slider/src/components/Slider/__snapshots__/Slider.test.tsx.snap # packages/react-slider/src/components/Slider/renderSlider.tsx # packages/react-slider/src/components/Slider/useSlider.ts # packages/react-slider/src/components/Slider/useSliderState.tsx # packages/react-slider/src/components/Slider/useSliderStyles.ts
…eat/slider-mark # Conflicts: # packages/react-slider/package.json
Pull request checklist
$ yarn change
Description of changes
Adding the
marks
prop to the Slider component.Behavior
The
marks
prop accepts three cases:Boolean
: If true marks are visible.number[]
: Marks will be displayed at each provided number.{value: number; label?: string | JSX.Element; mark?: JSX.Element}
: Marks are shown at the value location and display the custom label.Both cases two and three function together, meaning that you can pass in an array such as:
Custom JSX elements are also allowed to override the default marks and labels.
Example
CodeSandbox Demo
https://codesandbox.io/s/marked-slider-component-q5bpd?file=/src/App.js