Skip to content
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.

Sign up for GitHub

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

Jump to bottom

(react-slider) - Updating onChange typing #19412

Merged
merged 5 commits into from
Aug 18, 2021
Merged

(react-slider) - Updating onChange typing #19412

merged 5 commits into from
Aug 18, 2021

Conversation

czearing
Copy link
Collaborator

@czearing czearing commented Aug 17, 2021 •
edited
Loading

Pull request checklist

Description of changes

  1. Updating the onChange typing in the Slider component to follow:
(ev: React.PointerEvent<HTMLDivElement> | React.KeyboardEvent<HTMLDivElement> data: { value: number }) => void
  1. Updating tests and stories to work with the change.
  2. Removing useMount that was used to unnecessarily clamp user provided values during the initial render.
  3. Removing unnecessary test for clamping user provided controlled values.
  4. Updating the updateValue method to use the useEventCallback hook.

@czearing czearing requested a review from ecraig12345 August 17, 2021 03:16
@czearing czearing requested a review from a team as a code owner August 17, 2021 03:16
@fabricteam
Copy link
Collaborator

fabricteam commented Aug 17, 2021 •
edited
Loading

📊 Bundle size report

🤖 This report was generated against 74edc9eef50ee89471beee2ef8fb724f470854fc

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 17, 2021 •
edited
Loading

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 044c49e:

Sandbox Source
Fluent UI React Starter Configuration

@size-auditor
Copy link

size-auditor bot commented Aug 17, 2021 •
edited
Loading

Asset size changes

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

Baseline commit: 74edc9eef50ee89471beee2ef8fb724f470854fc (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Aug 17, 2021 •
edited
Loading

Perf Analysis (@fluentui/react)

Scenario Render type Master Ticks PR Ticks Iterations Status
Panel mount 2190 1498 1000 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 961 953 5000
BaseButton mount 979 984 5000
Breadcrumb mount 2653 2694 1000
ButtonNext mount 501 459 5000
Checkbox mount 1703 1578 5000
CheckboxBase mount 1444 1483 5000
ChoiceGroup mount 5267 5383 5000
ComboBox mount 1006 1042 1000
CommandBar mount 10409 10659 1000
ContextualMenu mount 6386 6318 1000
DefaultButton mount 1213 1175 5000
DetailsRow mount 3964 3967 5000
DetailsRowFast mount 4101 4074 5000
DetailsRowNoStyles mount 3731 3751 5000
Dialog mount 2208 2155 1000
DocumentCardTitle mount 177 156 1000
Dropdown mount 3522 3453 5000
FluentProviderNext mount 7125 7120 5000
FocusTrapZone mount 1897 1933 5000
FocusZone mount 1861 1902 5000
IconButton mount 1910 1889 5000
Label mount 368 361 5000
Layer mount 1877 1931 5000
Link mount 494 491 5000
MakeStyles mount 1819 1906 50000
MenuButton mount 1566 1572 5000
MessageBar mount 2082 2044 5000
Nav mount 3659 3613 1000
OverflowSet mount 1230 1228 5000
Panel mount 2190 1498 1000 Possible regression
Persona mount 925 918 1000
Pivot mount 1457 1532 1000
PrimaryButton mount 1360 1380 5000
Rating mount 8382 8263 5000
SearchBox mount 1483 1456 5000
Shimmer mount 2719 2793 5000
Slider mount 2113 2067 5000
SpinButton mount 5324 5475 5000
Spinner mount 440 416 5000
SplitButton mount 3314 3350 5000
Stack mount 520 542 5000
StackWithIntrinsicChildren mount 1663 1674 5000
StackWithTextChildren mount 5019 4911 5000
SwatchColorPicker mount 10780 10959 5000
Tabs mount 1455 1455 1000
TagPicker mount 2707 2727 5000
TeachingBubble mount 12118 12174 5000
Text mount 451 449 5000
TextField mount 1488 1516 5000
ThemeProvider mount 1249 1221 5000
ThemeProvider virtual-rerender 634 648 5000
Toggle mount 908 905 5000
buttonNative mount 127 132 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AttachmentMinimalPerf.default 179 164 1.09:1
VideoMinimalPerf.default 708 651 1.09:1
FlexMinimalPerf.default 322 299 1.08:1
TreeWith60ListItems.default 192 177 1.08:1
ChatDuplicateMessagesPerf.default 315 297 1.06:1
ChatWithPopoverPerf.default 392 371 1.06:1
SegmentMinimalPerf.default 394 371 1.06:1
BoxMinimalPerf.default 382 363 1.05:1
GridMinimalPerf.default 368 350 1.05:1
ListMinimalPerf.default 563 535 1.05:1
TreeMinimalPerf.default 880 837 1.05:1
AccordionMinimalPerf.default 166 160 1.04:1
AlertMinimalPerf.default 308 297 1.04:1
LoaderMinimalPerf.default 748 716 1.04:1
SkeletonMinimalPerf.default 388 373 1.04:1
DatepickerMinimalPerf.default 5787 5600 1.03:1
DropdownManyItemsPerf.default 741 720 1.03:1
HeaderMinimalPerf.default 398 388 1.03:1
PopupMinimalPerf.default 607 591 1.03:1
TableManyItemsPerf.default 2149 2085 1.03:1
TextMinimalPerf.default 387 375 1.03:1
TextAreaMinimalPerf.default 538 523 1.03:1
CarouselMinimalPerf.default 496 488 1.02:1
CheckboxMinimalPerf.default 2928 2884 1.02:1
DialogMinimalPerf.default 809 794 1.02:1
MenuButtonMinimalPerf.default 1756 1720 1.02:1
StatusMinimalPerf.default 737 723 1.02:1
IconMinimalPerf.default 659 645 1.02:1
ButtonOverridesMissPerf.default 1791 1780 1.01:1
EmbedMinimalPerf.default 4308 4262 1.01:1
InputMinimalPerf.default 1323 1313 1.01:1
MenuMinimalPerf.default 912 902 1.01:1
PortalMinimalPerf.default 177 176 1.01:1
RadioGroupMinimalPerf.default 485 480 1.01:1
SliderMinimalPerf.default 1642 1632 1.01:1
CustomToolbarPrototype.default 3984 3951 1.01:1
AttachmentSlotsPerf.default 1135 1133 1:1
AvatarMinimalPerf.default 203 203 1:1
ButtonMinimalPerf.default 189 189 1:1
ButtonSlotsPerf.default 579 578 1:1
ProviderMinimalPerf.default 1043 1043 1:1
RefMinimalPerf.default 229 229 1:1
SplitButtonMinimalPerf.default 4215 4201 1:1
TableMinimalPerf.default 415 417 1:1
ChatMinimalPerf.default 695 701 0.99:1
FormMinimalPerf.default 452 458 0.99:1
ImageMinimalPerf.default 400 403 0.99:1
ListWith60ListItems.default 686 693 0.99:1
ProviderMergeThemesPerf.default 1657 1672 0.99:1
TooltipMinimalPerf.default 1071 1077 0.99:1
CardMinimalPerf.default 580 594 0.98:1
ReactionMinimalPerf.default 417 425 0.98:1
DividerMinimalPerf.default 385 397 0.97:1
DropdownMinimalPerf.default 3108 3190 0.97:1
ItemLayoutMinimalPerf.default 1308 1347 0.97:1
ListCommonPerf.default 675 696 0.97:1
ListNestedPerf.default 604 625 0.97:1
RosterPerf.default 1266 1301 0.97:1
HeaderSlotsPerf.default 834 871 0.96:1
LayoutMinimalPerf.default 387 403 0.96:1
ToolbarMinimalPerf.default 988 1024 0.96:1
LabelMinimalPerf.default 396 418 0.95:1
AnimationMinimalPerf.default 431 466 0.92:1

@ecraig12345
Copy link
Member

Should have mentioned, this is the RFC for reference: https://github.com/microsoft/fluentui/blob/master/rfcs/convergence/event-handlers-arguments.md

ecraig12345
ecraig12345 reviewed Aug 17, 2021
View reviewed changes
packages/react-slider/src/components/Slider/Slider.test.tsx
@@ -164,7 +138,6 @@ describe('Slider', () => {
fireEvent.pointerDown(sliderRoot, { clientX: 0, clientY: 0 });

expect(onChange).toHaveBeenCalledTimes(1);
expect(onChange.mock.calls[0][0]).toEqual(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could change this to expect(onChange.mock.calls[0][1]).toEqual({ value: 0 }) and it would still work (similar elsewhere)

ecraig12345
ecraig12345 approved these changes Aug 18, 2021
View reviewed changes
Copy link
Member

@ecraig12345 ecraig12345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates!

@czearing czearing merged commit e6d5658 into microsoft:master Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@layershifter layershifter layershifter approved these changes

@khmakoto khmakoto khmakoto approved these changes

@ecraig12345 ecraig12345 ecraig12345 approved these changes

Assignees

@smhigley smhigley

Labels
Component: Slider
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@czearing @fabricteam @ecraig12345 @khmakoto @layershifter @smhigley @msft-fluent-ui-bot