High contrast: Multiple component support for mouse over states#4728
High contrast: Multiple component support for mouse over states#4728lynamemi merged 26 commits intomicrosoft:masterfrom
Conversation
|
Just adding a note that command bar has been promoted from experiments to component in 6.0 (presently in the |
|
@cliffkoh Is that something you want me to do or are you doing a sweep of those updates? |
|
Could you split the command bar changes out from this PR and create a separate PR for the command bar in new_6.0? Fortunately, the change looks simple to port. Amongst other things (such as 6.0 specific features/changes do not keep getting delayed further and further back due to a ceaseless stream of port-work), this will allow the commit moving forward to be correctly attributed to you :) Thank you! |
betrue-final-final
left a comment
betrue-final-final
left a comment
There was a problem hiding this comment.
Just reviewed with Emily. Looks good!
| color: semanticColors.linkHovered, | ||
| selectors: { | ||
| [HighContrastSelector]: { | ||
| textDecoration: 'underline' |
There was a problem hiding this comment.
Can you change this to border-bottom instead?
The reason is, I think for places where we're adding borders, we should just change it to be transparent borders and remove the high contrast selector. That way, it doesn't interfere with the default UX, while at the same time it'll actually be visible in high contrast mode in FireFox.
@betrue-final-final thoughts?
| [HighContrastSelector]: { | ||
| borderColor: 'Highlight' | ||
| borderColor: 'Highlight', | ||
| outline: 'none' |
There was a problem hiding this comment.
why not keep using outline here?
There was a problem hiding this comment.
The outline was creating a lot of noise, and Ben wanted it gone. See this screenshot:
phkuo
left a comment
phkuo
left a comment
There was a problem hiding this comment.
This is great! But I just want to have the discussion on borders for supporting FF before we continue. We did commit to supporting FireFox when reasonable, but high contrast selectors only work in IE.
|
Removing from here would result in the least merge conflict :) |
|
Thank you 👏 |
cliffkoh
left a comment
cliffkoh
left a comment
There was a problem hiding this comment.
Approved based on the very specific/limited set of things I was looking at 😄
| @@ -0,0 +1,11 @@ | |||
| { | |||
There was a problem hiding this comment.
There is no longer the need for this change file.
| @include high-contrast { | ||
| &::before { | ||
| box-sizing: border-box; | ||
| border-bottom: 2px solid Highlight; |
There was a problem hiding this comment.
set this to be a permanent hover style outside of @include high-contrast, and then change the color for just IE
* pass through className * pass className through to CommandButton as well * change request file * use getNativeProps * get rid of unnecessary newline * added Pivot snapshot test for custom className * remove space * Update snapshots due to merge of #4728. * Update pivotOverrideStyles_2018-04-03-18-42.json
5 participants
Pull request checklist
$ npm run changeDescription of changes
I grouped some of the simpler component states from the issue:
Breadcrumb:
Button (made this general change across many buttons):
Link:
Pivot:
SearchBox:
SpinButton:
Focus areas to test
(optional)