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

Button: fix size mismatch between primary and default buttons #11366

Merged
merged 7 commits into from
Dec 9, 2019

Conversation

xugao
Copy link
Contributor

@xugao xugao commented Dec 4, 2019

Pull request checklist

Description of changes

Fix bug where primary & default buttons with same child have different sizes due to primary one has no border but default one does.

Microsoft Reviewers: Open in CodeFlow

@size-auditor
Copy link

size-auditor bot commented Dec 4, 2019

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react TeachingBubble 189.864 kB 190.076 kB ExceedsBaseline     212 bytes
office-ui-fabric-react Button 179.268 kB 179.48 kB ExceedsBaseline     212 bytes

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

Baseline commit: 9f3391062f8f862986c54612a25c6702ebc6cf36 (build)

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Dec 4, 2019

Component Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Status
BaseButton 803 821
BaseButton (experiments) 1057 1049
DefaultButton 1042 1112
DefaultButton (experiments) 2045 2043
DetailsRow 3419 3349
DetailsRow (fast icons) 3426 3412
DetailsRow without styles 3182 3206
DocumentCardTitle with truncation 33547 33232
MenuButton 1376 1361
MenuButton (experiments) 3615 3579
PrimaryButton 1208 1289
PrimaryButton (experiments) 2076 2093
SplitButton 2962 2961
SplitButton (experiments) 7354 7290
Stack 525 528
Stack with Intrinsic children 1159 1157
Stack with Text children 4558 4501
Text 401 409
Toggle 893 888
Toggle (experiments) 2423 2439
button 62 63

@micahgodbolt
Copy link
Member

this is causing a small change with high contrast mode

before

image

after

image

Can you make sure that borders are picking up the high contrast background as well

@micahgodbolt
Copy link
Member

Also, the change in border is affecting the appearance of the focus state

before:

image

after

image

Not 100% sure the 'after' there is wrong. but it's a change. @betrue-final-final what's your take on the focus styles with that extra pixel border?

@betrue-final-final
Copy link
Member

It actually makes it a little clearer. I'm good with this. Would Default also look like this with the extra padding?

@micahgodbolt
Copy link
Member

better view of side by side
image

@micahgodbolt
Copy link
Member

Ben is good with the new focus rec......i'd say just clean up high contrast and we're good

@betrue-final-final
Copy link
Member

Forgot to hit send. Yes this is great.

@xugao xugao closed this Dec 9, 2019
@xugao xugao reopened this Dec 9, 2019
@khmakoto
Copy link
Member

khmakoto commented Dec 9, 2019

@xugao I think we also need to adjust the border of primary buttons in High Contrast when the buttons are hovered and pressed.

@micahgodbolt
Copy link
Member

high contrast is looking great now

image

@xugao xugao merged commit 19eabd5 into microsoft:master Dec 9, 2019
@msft-github-bot
Copy link
Contributor

🎉office-ui-fabric-react@v7.70.0 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Primary buttons are smaller normal buttons because they have no border
7 participants