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

Fix index import for Stack in TeachingBubbleContent #10988

Merged
merged 4 commits into from
Oct 30, 2019
Merged

Fix index import for Stack in TeachingBubbleContent #10988

merged 4 commits into from
Oct 30, 2019

Conversation

KevinTCoughlin
Copy link
Member

@KevinTCoughlin KevinTCoughlin commented Oct 30, 2019 •
edited
Loading

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ yarn change

Description of changes

This office-ui-fabric-react root import is causing a downstream partner's AMD bundle size to increase by ~160kB.

Focus areas to test

  • CI to pass
Microsoft Reviewers: Open in CodeFlow

ecraig12345
ecraig12345 approved these changes Oct 30, 2019
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.

If this is such a problem for AMD bundle size, we should look into adding a lint rule to prevent it from happening again. (Auto-added imports will often be in this format.)

@ecraig12345
Copy link
Member

By "lint rule" I mean another condition in the lint-imports script probably

@KevinTCoughlin
Copy link
Member Author

If this is such a problem for AMD bundle size, we should look into adding a lint rule to prevent it from happening again. (Auto-added imports will often be in this format.)

I agree. It would be extremely useful for AMD consumers. I don't have cycles to do the work until I finish internal upgrade work, but will consider it after that. I'm also willing to review a PR that adds such a rule :).

@msft-github-bot
Copy link
Contributor

Hello @KevinTCoughlin!

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.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 7 hours 50 minutes. No worries though, I will be back when the time is right! 😉

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.

@size-auditor
Copy link

size-auditor bot commented Oct 30, 2019

Asset size changes

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

Baseline commit: 0a42bf6702653658da3d7a61cd774f955136f450 (build)

@aditima aditima merged commit fb1958f into microsoft:master Oct 30, 2019
@KevinTCoughlin KevinTCoughlin deleted the keco/fix-index-import-for-stack branch October 30, 2019 00:49
@ecraig12345 ecraig12345 mentioned this pull request Oct 30, 2019
Closed
@msft-github-bot
Copy link
Contributor

Component Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Status
BaseButton 757 779
BaseButton (experiments) 1008 1064
DefaultButton 1029 1027
DefaultButton (experiments) 2026 1965
DetailsRow 3357 3370
DetailsRow (fast icons) 3355 3304
DetailsRow without styles 3050 3023
DocumentCardTitle with truncation 31554 31638
MenuButton 1370 1339
MenuButton (experiments) 3645 3687
PrimaryButton 1233 1200
PrimaryButton (experiments) 2102 2059
SplitButton 2840 2842
SplitButton (experiments) 7303 7277
Stack 504 509
Stack with Intrinsic children 1200 1186
Stack with Text children 4503 4420
Text 389 402
Toggle 847 837
Toggle (experiments) 2378 2391
button 67 47

@msft-github-bot
Copy link
Contributor

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ecraig12345 ecraig12345 ecraig12345 approved these changes

@aditima aditima aditima approved these changes

@dzearing dzearing Awaiting requested review from dzearing

@marygans marygans Awaiting requested review from marygans

@micahgodbolt micahgodbolt Awaiting requested review from micahgodbolt

Assignees

@jdhuntington jdhuntington

Labels
AutoMerge Component: TeachingBubble
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@KevinTCoughlin @ecraig12345 @msft-github-bot @aditima @jdhuntington