<Fabric> element does not affect global button styling. by jdhuntington · Pull Request #9076 · microsoft/fluentui

jdhuntington · 2019-05-13T18:27:28Z

Pull request checklist

Addresses an existing issue: Fixes Global styles applied to "button" #9036
Include a change request file using $ npm run change

Description of changes

<Fabric> performed a global style reset of <button> in order to fix #3691 . This change reduces that scope of reset to elements within a <Fabric> element.

Note that packages/office-ui-fabric-react/src/components/Fabric/Fabric.styles.ts is the only code change; all other modified files are snapshots.

Microsoft Reviewers: Open in CodeFlow

msft-github-bot · 2019-05-13T18:48:13Z

Component perf results:

Scenario	Target branch avg total (ms)	PR avg total (ms)	Target branch avg per item (ms)	PR avg per item (ms)	Is significant change	Is regression
PrimaryButton	73.766	76.795	0.738	0.768	false	false
BaseButton	29.263	30.942	0.293	0.309	false	false
NewButton	105.939	106.172	1.059	1.062	false	false
button	4.843	4.846	0.048	0.048	false	false
DetailsRows without styles	166.768	167.293	1.668	1.673	false	false
DetailsRows	191.610	192.755	1.916	1.928	false	false
Toggles	45.243	45.547	0.452	0.455	false	false
NewToggle	65.503	65.164	0.655	0.652	false	false
DocumentCardTitle with truncation	23.866	24.227	0.239	0.242	false	false

size-auditor · 2019-05-13T18:55:46Z

Bundle test	Size (minified)	Diff from master
MessageBar	175.036 kB	2 bytes
Pickers	259.362 kB	2 bytes
Dropdown	215.311 kB	2 bytes
DocumentCard	197.405 kB	2 bytes
SearchBox	172.088 kB	2 bytes
SelectedItemsList	209.787 kB	2 bytes
Dialog	194.13 kB	2 bytes
CommandBar	185.564 kB	2 bytes
ComboBox	223.784 kB	2 bytes
SpinButton	178.379 kB	2 bytes
Panel	184.105 kB	2 bytes
TeachingBubble	180.054 kB	2 bytes
Button	177.479 kB	2 bytes
Breadcrumb	184.084 kB	2 bytes
FloatingPicker	220.552 kB	2 bytes
Facepile	193.774 kB	-39 bytes
Nav	174.639 kB	-39 bytes
Pivot	173.633 kB	-39 bytes
SwatchColorPicker	179.358 kB	-39 bytes
Grid	168.303 kB	-39 bytes
Persona	113.499 kB	-48 bytes
Tooltip	88.762 kB	-48 bytes
KeytipLayer	102.384 kB	-48 bytes
Callout	83.354 kB	-48 bytes
Keytip	79.53 kB	-48 bytes
HoverCard	99.005 kB	-48 bytes
PersonaCoin	113.499 kB	-48 bytes
Coachmark	97.505 kB	-48 bytes
Fabric	39.555 kB	-48 bytes
ShimmeredDetailsList	225.95 kB	-48 bytes
ContextualMenu	148.95 kB	-48 bytes
DatePicker	203.63 kB	-48 bytes
DetailsList	214.952 kB	-48 bytes
Modal	93.755 kB	-48 bytes
PositioningContainer	76.573 kB	-48 bytes
Layer	49.231 kB	-48 bytes

Exceeds Tolerance Exceeds Baseline Below Baseline 1 kB = 1000 bytes

Vitalius1 · 2019-05-13T23:01:47Z

@jdhuntington screener is showing a visible change to the Calendar. Might be worth investigating before merging. Nothing else changed so it's probably something specific only to Calendar.

jdhuntington · 2019-05-13T23:43:23Z

Looks like the specificity of the selector caused by this change is going to cause more problems down the road. Going to try another approach.

Vitalius1 · 2019-05-14T02:36:08Z

@jdhuntington there is a PR (#9037) dealing with the exact same issue. Which one needs to be kept open?

hoovercj · 2019-05-15T07:33:16Z

@Vitalius1 I opened #9037 only as a reference. I have now closed it in favor of this pull request.

hoovercj · 2019-05-15T07:39:19Z

Looks like the specificity of the selector caused by this change is going to cause more problems down the road. Going to try another approach.

@jdhuntington - there was a comment in the initial PR by @micahgodbolt regarding specificity issues of different solutions.

msft-github-bot · 2019-05-15T23:04:33Z

Hello @jdhuntington!

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 and give me an instruction to get started! Learn more here.

khmakoto

Left a little comment but otherwise LGTM 👍

khmakoto · 2019-05-16T17:56:32Z

common/changes/@uifabric/experiments/fix-button-global-styling_2019-05-15-18-24.json

+    {
+      "packageName": "@uifabric/experiments",
+      "comment": "<Fabric> element does not affect global button styling.",
+      "type": "minor"


Shouldn't these be of type patch since this is not changing the API surface?

I think minor is appropriate as it will affect global styles for anyone that uses the <Fabric> element.

msft-github-bot · 2019-05-21T12:23:08Z

🎉@uifabric/experiments@v6.77.0 has been released which incorporates this pull request.:tada:

Handy links:

msft-github-bot · 2019-05-21T12:23:13Z

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

Handy links:

jdhuntington requested review from Jahnp, JasonGore, ThomasMichon, Vitalius1, atneik, dzearing, kkjeer and micahgodbolt as code owners May 13, 2019 18:27

msft-github-bot assigned jdhuntington May 13, 2019

Vitalius1 approved these changes May 13, 2019

View reviewed changes

dzearing approved these changes May 13, 2019

View reviewed changes

Jahnp approved these changes May 13, 2019

View reviewed changes

jdhuntington added the Status: Do Not Merge 🚫 label May 13, 2019

ecraig12345 added the Component: Button label May 13, 2019

jdhuntington force-pushed the fix-button-global-styling branch from a9d8fa3 to 14ab063 Compare May 14, 2019 17:45

jdhuntington requested review from aneeshack4, ecraig12345, joschect and khmakoto as code owners May 14, 2019 17:45

joschect approved these changes May 14, 2019

View reviewed changes

ecraig12345 approved these changes May 14, 2019

View reviewed changes

jdhuntington force-pushed the fix-button-global-styling branch from 14ab063 to f5e6054 Compare May 14, 2019 23:46

Fabric control doesn't affect global button style.

0919a1a

jdhuntington force-pushed the fix-button-global-styling branch from ac6ae8d to 0919a1a Compare May 15, 2019 18:27

jdhuntington added AutoMerge and removed Status: Do Not Merge 🚫 labels May 15, 2019

khmakoto approved these changes May 16, 2019

View reviewed changes

aneeshack4 approved these changes May 16, 2019

View reviewed changes

dzearing approved these changes May 17, 2019

View reviewed changes

jdhuntington merged commit 2bbb305 into microsoft:master May 20, 2019

This was referenced May 21, 2019

SplitButton: In Safari, the SplitButton has mismatched heights #3691

Closed

Global styles applied to "button" #9036

Closed

microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019

Conversation

jdhuntington commented May 13, 2019 • edited by msft-github-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Microsoft Reviewers: Open in CodeFlow

Uh oh!

msft-github-bot commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

size-auditor bot commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vitalius1 commented May 13, 2019

Uh oh!

jdhuntington commented May 13, 2019

Uh oh!

Vitalius1 commented May 14, 2019

Uh oh!

hoovercj commented May 15, 2019

Uh oh!

hoovercj commented May 15, 2019

Uh oh!

msft-github-bot commented May 15, 2019

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 and give me an instruction to get started! Learn more here.

Uh oh!

khmakoto left a comment

Choose a reason for hiding this comment

Uh oh!

khmakoto May 16, 2019

Choose a reason for hiding this comment

Uh oh!

jdhuntington May 17, 2019

Choose a reason for hiding this comment

Uh oh!

msft-github-bot commented May 21, 2019

Uh oh!

msft-github-bot commented May 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

jdhuntington commented May 13, 2019 •

edited by msft-github-bot

Loading

msft-github-bot commented May 13, 2019 •

edited

Loading

size-auditor bot commented May 13, 2019 •

edited

Loading