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.

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

Fix input and label IDs not respected by choice group option. #14387

Merged
merged 4 commits into from
Aug 7, 2020

Conversation

frags51
Copy link
Contributor

@frags51 frags51 commented Aug 6, 2020

Pull request checklist

Description of changes

Passed the correct input and label id's to ChoiceGroupOption.

Focus areas to test

Please refer to the issue linked above for more details.

@ghost
Copy link

ghost commented Aug 6, 2020

CLA assistant check
All CLA requirements met.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 6, 2020

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 92e9a09:

Sandbox Source
Fluent UI Button Configuration
microsoft/fluentui: codesandbox-react-template Configuration
microsoft/fluentui: codesandbox-react-next-template Configuration
microsoft/fluentui: codesandbox-react-northstar-template Configuration

@size-auditor
Copy link

size-auditor bot commented Aug 6, 2020

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react office-ui-fabric-react-ChoiceGroup 59.953 kB 60.046 kB ExceedsBaseline     93 bytes
office-ui-fabric-react fluentui-react-next-ChoiceGroup 59.953 kB 60.046 kB ExceedsBaseline     93 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: 7db0feacd08336094a1fec9f857621342f5303c7 (build)

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Aug 6, 2020

Perf Analysis

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 911 867 5000
ButtonNext mount 599 575 5000
Checkbox mount 1542 1513 5000
CheckboxBase mount 1296 1328 5000
CheckboxNext mount 1611 1620 5000
ChoiceGroup mount 5037 4884 5000
ComboBox mount 904 901 1000
CommandBar mount 8236 7866 1000
ContextualMenu mount 15783 15986 1000
DefaultButton mount 1062 1107 5000
DetailsRow mount 3525 3501 5000
DetailsRowFast mount 3686 3625 5000
DetailsRowNoStyles mount 3499 3355 5000
Dialog mount 1506 1467 1000
DocumentCardTitle mount 1832 1875 1000
Dropdown mount 2548 2604 5000
FocusZone mount 1875 1854 5000
IconButton mount 1726 1747 5000
Label mount 340 361 5000
Link mount 480 471 5000
LinkNext mount 500 495 5000
MenuButton mount 1426 1414 5000
Nav mount 3231 3290 1000
Panel mount 1468 1470 1000
Persona mount 841 843 1000
Pivot mount 1450 1435 1000
PivotNext mount 1531 1389 1000
PrimaryButton mount 1322 1292 5000
SearchBox mount 1259 1293 5000
SearchBoxNext mount 1319 1366 5000
Slider mount 1520 1502 5000
SliderNext mount 1895 1948 5000
SpinButton mount 4946 4960 5000
SpinButtonNext mount 5033 4960 5000
Spinner mount 384 426 5000
SplitButton mount 3075 3035 5000
Stack mount 541 516 5000
StackWithIntrinsicChildren mount 1988 2028 5000
StackWithTextChildren mount 5004 5008 5000
TagPicker mount 2742 2730 5000
Text mount 436 419 5000
TextField mount 1378 1403 5000
ThemeProvider mount 2897 3140 5000
ThemeProvider virtual-rerender 471 478 5000
Toggle mount 812 838 5000
ToggleNext mount 802 848 5000
button mount 118 131 5000

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🎯 Avatar.Fluent 0.42 0.47 0.89:1 2000 843
🦄 Button.Fluent 0.11 0.19 0.58:1 5000 543
🔧 Checkbox.Fluent 0.65 0.35 1.86:1 1000 646
🎯 Dialog.Fluent 0.15 0.21 0.71:1 5000 746
🔧 Dropdown.Fluent 2.92 0.47 6.21:1 1000 2924
🔧 Icon.Fluent 0.14 0.05 2.8:1 5000 702
🦄 Image.Fluent 0.07 0.11 0.64:1 5000 363
🔧 Slider.Fluent 1.62 0.35 4.63:1 1000 1618
🔧 Text.Fluent 0.07 0.03 2.33:1 5000 329
🦄 Tooltip.Fluent 0.1 20.43 0:1 5000 522

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
RefMinimalPerf.default 232 211 1.1:1
CarouselMinimalPerf.default 470 430 1.09:1
MenuMinimalPerf.default 844 787 1.07:1
Checkbox.Fluent 646 610 1.06:1
ChatWithPopoverPerf.default 496 474 1.05:1
DropdownManyItemsPerf.default 756 718 1.05:1
SkeletonMinimalPerf.default 389 370 1.05:1
IconMinimalPerf.default 672 643 1.05:1
TableMinimalPerf.default 391 371 1.05:1
AccordionMinimalPerf.default 151 145 1.04:1
AlertMinimalPerf.default 299 287 1.04:1
GridMinimalPerf.default 318 305 1.04:1
LabelMinimalPerf.default 395 381 1.04:1
RadioGroupMinimalPerf.default 395 379 1.04:1
TextAreaMinimalPerf.default 451 433 1.04:1
TreeMinimalPerf.default 859 825 1.04:1
VideoMinimalPerf.default 619 598 1.04:1
ChatMinimalPerf.default 606 586 1.03:1
DividerMinimalPerf.default 345 336 1.03:1
ListMinimalPerf.default 471 457 1.03:1
SegmentMinimalPerf.default 343 333 1.03:1
TextMinimalPerf.default 339 329 1.03:1
Icon.Fluent 702 683 1.03:1
AnimationMinimalPerf.default 399 390 1.02:1
AvatarMinimalPerf.default 465 457 1.02:1
ButtonSlotsPerf.default 604 594 1.02:1
ListNestedPerf.default 895 879 1.02:1
LoaderMinimalPerf.default 742 724 1.02:1
ReactionMinimalPerf.default 374 368 1.02:1
Slider.Fluent 1618 1588 1.02:1
CardMinimalPerf.default 538 535 1.01:1
EmbedMinimalPerf.default 1838 1826 1.01:1
FormMinimalPerf.default 380 375 1.01:1
HierarchicalTreeMinimalPerf.default 408 402 1.01:1
ProviderMergeThemesPerf.default 1925 1909 1.01:1
SplitButtonMinimalPerf.default 3739 3704 1.01:1
Button.Fluent 543 540 1.01:1
Text.Fluent 329 325 1.01:1
InputMinimalPerf.default 1311 1313 1:1
ListCommonPerf.default 933 929 1:1
SliderMinimalPerf.default 1615 1612 1:1
TooltipMinimalPerf.default 775 772 1:1
TreeWith60ListItems.default 216 215 1:1
Image.Fluent 363 362 1:1
AttachmentMinimalPerf.default 154 156 0.99:1
CheckboxMinimalPerf.default 2836 2864 0.99:1
HeaderMinimalPerf.default 343 346 0.99:1
PopupMinimalPerf.default 669 675 0.99:1
TableManyItemsPerf.default 2130 2155 0.99:1
CustomToolbarPrototype.default 3778 3826 0.99:1
ToolbarMinimalPerf.default 925 931 0.99:1
Dropdown.Fluent 2924 2942 0.99:1
Tooltip.Fluent 522 529 0.99:1
BoxMinimalPerf.default 319 326 0.98:1
ButtonMinimalPerf.default 160 163 0.98:1
ChatDuplicateMessagesPerf.default 427 436 0.98:1
FlexMinimalPerf.default 270 276 0.98:1
ImageMinimalPerf.default 345 353 0.98:1
ItemLayoutMinimalPerf.default 1218 1244 0.98:1
ListWith60ListItems.default 1075 1098 0.98:1
MenuButtonMinimalPerf.default 1483 1514 0.98:1
ProviderMinimalPerf.default 926 948 0.98:1
StatusMinimalPerf.default 639 655 0.98:1
Avatar.Fluent 843 861 0.98:1
DialogMinimalPerf.default 727 747 0.97:1
HeaderSlotsPerf.default 754 778 0.97:1
LayoutMinimalPerf.default 369 380 0.97:1
Dialog.Fluent 746 772 0.97:1
AttachmentSlotsPerf.default 1106 1158 0.96:1
PortalMinimalPerf.default 117 122 0.96:1
DropdownMinimalPerf.default 2927 3107 0.94:1

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 this fix! A couple minor syntax suggestions but otherwise looks good.

@czearing Can you please include this change in your ChoiceGroup function component conversion PR too?

frags51 and others added 2 commits August 7, 2020 10:38
…iceGroup.base.tsx

Co-authored-by: Elizabeth Craig <ecraig12345@gmail.com>
…iceGroup.base.tsx

Co-authored-by: Elizabeth Craig <ecraig12345@gmail.com>
@frags51
Copy link
Contributor Author

frags51 commented Aug 7, 2020

@ecraig12345 Done!
Edit: Any idea why the build is failing now?

@ecraig12345
Copy link
Member

Not sure why the build was failing, but there's no way it was due to your change. I started a new build.

@msft-github-bot
Copy link
Contributor

Hello @ecraig12345!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msft-github-bot) and give me an instruction to get started! Learn more here.

@msft-github-bot msft-github-bot merged commit 815d67e into microsoft:master Aug 7, 2020
@msft-github-bot
Copy link
Contributor

🎉office-ui-fabric-react@v7.126.3 has been released which incorporates this pull request.:tada:

Handy links:

tmaster628 pushed a commit to tmaster628/fluentui that referenced this pull request Aug 12, 2020
…oft#14387)

#### Pull request checklist

- [x] Addresses an existing issue: Fixes microsoft#14386
- [x] Include a change request file using `$ yarn change`

#### Description of changes

Passed the correct input and label id's to ChoiceGroupOption.

#### Focus areas to test

Please refer to the issue linked above for more details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ChoiceGroup Component: ChoiceGroupOption does not respect custom 'id' and 'labelId' fields
4 participants