fix(Alert): add isFromKeyboard to state#1238
Conversation
layershifter
requested review from
dvdzkwsk,
kuzhelov,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1238 +/- ##
==========================================
+ Coverage 71.57% 71.58% +0.01%
==========================================
Files 730 730
Lines 5579 5582 +3
Branches 1634 1612 -22
==========================================
+ Hits 3993 3996 +3
Misses 1581 1581
Partials 5 5
Continue to review full report at Codecov.
|
| return ( | ||
| <ElementType | ||
| className={classes.root} | ||
| onFocus={this.handleFocus} |
There was a problem hiding this comment.
Shouldn't this be added on the action, instead of the root element?
There was a problem hiding this comment.
The Alert itself as far as I understand is not focusable
There was a problem hiding this comment.
Current points:
- as I see the whole
Alertis not focusable by spec, so currently onlyactionslot can be focused focusevent will be propogated from theactionslot
If we will have issues with this in future, i.e. the whole Alert can be focused we can separate values to isFromKeyboard & isAlertFromKeyboard.
There was a problem hiding this comment.
Make sense, I understand that it will be propagated, but my point was more in a sense that, if the content of the alert has focusable event, it would in the same way behave. But we can anyway use the ':focus' selector together with the isFromKeyboard state value, so it should be ok.
codepretty
left a comment
codepretty
left a comment
There was a problem hiding this comment.
Yay! Thanks so much for fixing this! :)
3 participants
Fixes #1230.