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

Remove default value for chat header slot #16321

Merged
merged 3 commits into from
Jan 4, 2021
Merged

Remove default value for chat header slot #16321

merged 3 commits into from
Jan 4, 2021

Conversation

ling1726
Copy link
Member

Pull request checklist

Description of changes

Removes the default value for the header shorthand slot in ChatMessage since it will unnecessarily throw warnings if the component is used with children

Warning: Failed prop type: Prop `author` in `ChatMessage` conflicts with props: `children`. They cannot be defined together, choose one or the other.

// Does not render as intended but will still unnecessarily throw even if no header prop is passed
<Chat.Message author="Jane Doe" timestamp="Yesterday, 10:15 PM" mine>
        What's up
</Chat.Message>

Focus areas to test

(optional)

@ling1726 ling1726 added the Fluent UI react-northstar (v0) Work related to Fluent UI V0 label Dec 30, 2020
@layershifter
Copy link
Member

Please add a condition to always render the header slot: const headerElement = createShorthand(ChatMessageHeader, header || {}, {

@codesandbox-ci
Copy link

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 90b6392:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration
fluent-ui-example (forked) Issue #16301

@fabricteam
Copy link
Collaborator

fabricteam commented Dec 30, 2020 •
edited
Loading

Perf Analysis

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 822 810 5000
BaseButtonCompat mount 875 866 5000
Breadcrumb mount 43658 43624 5000
Checkbox mount 1477 1473 5000
CheckboxBase mount 1203 1234 5000
ChoiceGroup mount 4566 4658 5000
ComboBox mount 1032 956 1000
CommandBar mount 10096 10056 1000
ContextualMenu mount 6015 6093 1000
DefaultButtonCompat mount 1089 1095 5000
DetailsRow mount 3565 3637 5000
DetailsRowFast mount 3652 3614 5000
DetailsRowNoStyles mount 3459 3376 5000
Dialog mount 1455 1426 1000
DocumentCardTitle mount 1859 1818 1000
Dropdown mount 3272 3268 5000
FocusTrapZone mount 1762 1792 5000
FocusZone mount 1791 1815 5000
IconButtonCompat mount 1723 1702 5000
Label mount 328 338 5000
Layer mount 1740 1715 5000
Link mount 465 453 5000
MakeStyles mount 1905 1944 50000
MenuButtonCompat mount 1415 1458 5000
MessageBar mount 1998 1968 5000
Nav mount 3197 3218 1000
OverflowSet mount 1042 1025 5000
Panel mount 1364 1406 1000
Persona mount 852 842 1000
Pivot mount 1389 1400 1000
PrimaryButtonCompat mount 1267 1242 5000
Rating mount 7342 7363 5000
SearchBox mount 1293 1306 5000
Shimmer mount 2461 2465 5000
Slider mount 1858 1911 5000
SpinButton mount 4866 4927 5000
Spinner mount 420 407 5000
SplitButtonCompat mount 3057 3058 5000
Stack mount 497 489 5000
StackWithIntrinsicChildren mount 1496 1525 5000
StackWithTextChildren mount 4403 4374 5000
SwatchColorPicker mount 10047 10005 5000
Tabs mount 1403 1362 1000
TagPicker mount 2797 2719 5000
TeachingBubble mount 11459 11537 5000
Text mount 398 425 5000
TextField mount 1352 1370 5000
ThemeProvider mount 2138 2138 5000
ThemeProvider virtual-rerender 627 637 5000
Toggle mount 790 803 5000
button mount 639 665 5000
buttonNative mount 110 113 5000

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🎯 Avatar.Fluent 0.44 0.51 0.86:1 2000 884
🦄 Button.Fluent 0.12 0.2 0.6:1 5000 581
🔧 Checkbox.Fluent 0.66 0.32 2.06:1 1000 659
🎯 Dialog.Fluent 0.16 0.21 0.76:1 5000 821
🔧 Dropdown.Fluent 3 0.38 7.89:1 1000 3000
🔧 Icon.Fluent 0.15 0.06 2.5:1 5000 747
🦄 Image.Fluent 0.08 0.13 0.62:1 5000 378
🔧 Slider.Fluent 1.6 0.45 3.56:1 1000 1596
🔧 Text.Fluent 0.07 0.03 2.33:1 5000 360
🦄 Tooltip.Fluent 0.12 0.87 0.14:1 5000 576

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
FlexMinimalPerf.default 330 294 1.12:1
PortalMinimalPerf.default 184 168 1.1:1
BoxMinimalPerf.default 387 361 1.07:1
ButtonMinimalPerf.default 191 179 1.07:1
FormMinimalPerf.default 438 408 1.07:1
GridMinimalPerf.default 374 351 1.07:1
Checkbox.Fluent 659 616 1.07:1
AttachmentMinimalPerf.default 169 160 1.06:1
TreeWith60ListItems.default 217 204 1.06:1
VideoMinimalPerf.default 661 625 1.06:1
CarouselMinimalPerf.default 458 440 1.04:1
Dialog.Fluent 821 788 1.04:1
Icon.Fluent 747 719 1.04:1
ButtonSlotsPerf.default 603 588 1.03:1
ChatWithPopoverPerf.default 482 467 1.03:1
DividerMinimalPerf.default 394 383 1.03:1
HeaderSlotsPerf.default 828 800 1.03:1
ListNestedPerf.default 594 576 1.03:1
SegmentMinimalPerf.default 377 366 1.03:1
StatusMinimalPerf.default 755 730 1.03:1
AccordionMinimalPerf.default 155 152 1.02:1
CardMinimalPerf.default 573 563 1.02:1
CheckboxMinimalPerf.default 2907 2864 1.02:1
EmbedMinimalPerf.default 4216 4128 1.02:1
ReactionMinimalPerf.default 429 421 1.02:1
SplitButtonMinimalPerf.default 3756 3692 1.02:1
IconMinimalPerf.default 693 681 1.02:1
TooltipMinimalPerf.default 833 818 1.02:1
AnimationMinimalPerf.default 413 410 1.01:1
HeaderMinimalPerf.default 386 384 1.01:1
ImageMinimalPerf.default 402 397 1.01:1
InputMinimalPerf.default 1310 1300 1.01:1
LoaderMinimalPerf.default 742 734 1.01:1
PopupMinimalPerf.default 711 702 1.01:1
TableMinimalPerf.default 434 431 1.01:1
TextAreaMinimalPerf.default 503 497 1.01:1
CustomToolbarPrototype.default 3980 3935 1.01:1
Dropdown.Fluent 3000 2982 1.01:1
Tooltip.Fluent 576 572 1.01:1
AvatarMinimalPerf.default 468 469 1:1
ButtonUseCssPerf.default 832 831 1:1
ChatMinimalPerf.default 629 627 1:1
DialogMinimalPerf.default 800 798 1:1
DropdownMinimalPerf.default 2994 2992 1:1
LabelMinimalPerf.default 433 431 1:1
ListMinimalPerf.default 503 505 1:1
RefMinimalPerf.default 246 247 1:1
SliderMinimalPerf.default 1589 1593 1:1
Avatar.Fluent 884 880 1:1
Slider.Fluent 1596 1590 1:1
ButtonOverridesMissPerf.default 1691 1706 0.99:1
ButtonUseCssNestingPerf.default 1095 1101 0.99:1
DatepickerMinimalPerf.default 46807 47075 0.99:1
DropdownManyItemsPerf.default 743 748 0.99:1
ItemLayoutMinimalPerf.default 1283 1290 0.99:1
LayoutMinimalPerf.default 424 427 0.99:1
ListCommonPerf.default 642 649 0.99:1
ListWith60ListItems.default 944 950 0.99:1
MenuButtonMinimalPerf.default 1579 1593 0.99:1
SkeletonMinimalPerf.default 377 380 0.99:1
TableManyItemsPerf.default 2093 2116 0.99:1
TreeMinimalPerf.default 800 806 0.99:1
Button.Fluent 581 588 0.99:1
MenuMinimalPerf.default 863 878 0.98:1
ProviderMinimalPerf.default 1021 1037 0.98:1
TextMinimalPerf.default 359 367 0.98:1
Text.Fluent 360 368 0.98:1
AttachmentSlotsPerf.default 1104 1136 0.97:1
ProviderMergeThemesPerf.default 2086 2158 0.97:1
ToolbarMinimalPerf.default 946 974 0.97:1
Image.Fluent 378 391 0.97:1
RadioGroupMinimalPerf.default 444 464 0.96:1
AlertMinimalPerf.default 298 318 0.94:1
ChatDuplicateMessagesPerf.default 408 442 0.92:1

@size-auditor
Copy link

size-auditor bot commented Dec 30, 2020 •
edited
Loading

Asset size changes

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

Baseline commit: 9a4995b0437ff05317f80b765e6b110b60a18cdb (build)

@ling1726 ling1726 closed this Dec 30, 2020
@ling1726 ling1726 reopened this Dec 30, 2020
@ling1726 ling1726 closed this Dec 31, 2020
@ling1726 ling1726 reopened this Dec 31, 2020
@ling1726 ling1726 merged commit 5902550 into microsoft:master Jan 4, 2021
@ling1726 ling1726 mentioned this pull request Jan 4, 2021
Merged
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

@assuncaocharles assuncaocharles Awaiting requested review from assuncaocharles

@dzearing dzearing Awaiting requested review from dzearing

@jurokapsiar jurokapsiar Awaiting requested review from jurokapsiar

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny

@pompomon pompomon Awaiting requested review from pompomon

Assignees

@ecraig12345 ecraig12345

Labels
Fluent UI react-northstar (v0) Work related to Fluent UI V0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ChatMessage throws error when used with children
4 participants
@ling1726 @layershifter @fabricteam @ecraig12345