fix(grid): move focus outline inside grid items #1195
Conversation
bmdalex
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1195 +/- ##
=======================================
Coverage 82.47% 82.47%
=======================================
Files 740 740
Lines 8754 8754
Branches 1170 1236 +66
=======================================
Hits 7220 7220
Misses 1519 1519
Partials 15 15
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1195 +/- ##
==========================================
- Coverage 82.48% 82.47% -0.01%
==========================================
Files 742 743 +1
Lines 8758 8761 +3
Branches 1236 1236
==========================================
+ Hits 7224 7226 +2
- Misses 1519 1520 +1
Partials 15 15
Continue to review full report at Codecov.
|
| @@ -24,6 +24,7 @@ const gridStyles: ComponentSlotStylesInput<GridProps, GridVariables> = { | |||
| gridGap, | |||
| display: 'grid', | |||
| justifyContent: 'space-evenly', | |||
| '& > *': { outlineOffset: '-3px' }, | |||
There was a problem hiding this comment.
as we've discussed, I am suggesting to introduce this as part of the Teams theme, as this is about Teams scenarios that have introduced this issue. For other scenarios we would just introduce the same behavior (and visual appearance) as plain browser styles would provide, so I am suggesting to stick to that base browser's one in the base theme
There was a problem hiding this comment.
oops...didn't notice it was moved to base; fixed, thanks, @kuzhelov
bmdalex
force-pushed
the
fix/grid-focus-outline
branch
from
dc0ce19
to
392e841
Compare
| @@ -43,31 +43,21 @@ const renderImages = () => { | |||
|
|
|||
| const renderImageButtons = () => { | |||
| return _.map(imageNames, imageName => ( | |||
| <Button key={imageName} styles={imageButtonStyles}> | |||
| <Button key={imageName} text styles={imageButtonStyles}> | |||
There was a problem hiding this comment.
would suggest to remove this Button's prop, however I do understand why it was introduced. Still, we shouldn't fine tune the examples because of the particular theme experiencing styling problems - in contrast, we'd better to make them visible.
Imagine what this change would be for any theme other than Teams - the semantics of the markup is not consistent (why text) and, most probably, will cause presentation problems to other theme, as it will style text Button differently than Teams theme - maybe this text Button will have a look drastically different from the default one.
So, based on this reasoning, suggesting to remove this prop from the docs example
There was a problem hiding this comment.
I removed it. We still need to understand if the fact that the Button's border goes outside the element on keyboard focus is a styling problem. We need to understand if placing Button components next to each other is a valid scenario for layouting as shown in: https://codesandbox.io/s/w2xoqpy23l
If that's true, this is a styling issue that need to be addressed in Button style file
fyi @bcalvery
fix(grid): move focus outline inside grid items
Fixes #1035
Before:
After: