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

DocumentCard: Adding explicit focus styles #11306

Merged
merged 4 commits into from Dec 5, 2019
Merged

DocumentCard: Adding explicit focus styles #11306

merged 4 commits into from Dec 5, 2019

Conversation

micahgodbolt
Copy link
Member

@micahgodbolt micahgodbolt commented Nov 25, 2019 •
edited

Pull request checklist

Description of changes

before chrome:

image

before edge:

image

after:

image

high contrast works nicely too:

image

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

@size-auditor
Copy link

size-auditor bot commented Nov 26, 2019 •
edited

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react DocumentCard 199.727 kB 199.97 kB ExceedsBaseline     243 bytes

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

Baseline commit: ed8a416b4628ea7c2ec8b53ef878422183d80f9d (build)

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Nov 26, 2019 •
edited

Component Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Status
BaseButton 712 735
BaseButton (experiments) 924 940
DefaultButton 937 915
DefaultButton (experiments) 1843 1883
DetailsRow 3099 3067
DetailsRow (fast icons) 3016 3145
DetailsRow without styles 2800 2924
DocumentCardTitle with truncation 29235 29046
MenuButton 1236 1248
MenuButton (experiments) 3249 3300
PrimaryButton 1077 1109
PrimaryButton (experiments) 1834 1860
SplitButton 2626 2612
SplitButton (experiments) 6604 6650
Stack 417 428
Stack with Intrinsic children 1017 1037
Stack with Text children 4030 4031
Text 355 347
Toggle 769 792
Toggle (experiments) 2140 2141
button 56 56

@micahgodbolt
Copy link
Member Author

@betrue-final-final would like some design review on this change. I used the existing border color for the focus rectangle. Easy to go darker or primary color if desired.

@betrue-final-final
Copy link
Member

@micahgodbolt, the focus rectangle should be no lighter than neutralSecondary. It looks too light in the screenshots here.

@micahgodbolt
Copy link
Member Author

@betrue-final-final color updated to neutral secondary

@msft-github-bot
Copy link
Contributor

Hello @micahgodbolt!

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.

betrue-final-final
betrue-final-final approved these changes Dec 5, 2019
View reviewed changes
Copy link
Member

@betrue-final-final betrue-final-final left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@micahgodbolt micahgodbolt merged commit da74700 into microsoft:master Dec 5, 2019
@micahgodbolt micahgodbolt deleted the doc-card-focus branch December 5, 2019 22:45
@khmakoto khmakoto mentioned this pull request Dec 5, 2019
Closed
@msft-github-bot
Copy link
Contributor

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

Handy links:

@msft-github-bot msft-github-bot mentioned this pull request Dec 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betrue-final-final betrue-final-final betrue-final-final approved these changes

@yiminwu yiminwu Awaiting requested review from yiminwu

Assignees

@aneeshack4 aneeshack4

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

Successfully merging this pull request may close these issues.

DocumentCard: Focus indicator is not properly visible
4 participants
@micahgodbolt @msft-github-bot @betrue-final-final @aneeshack4