-
Notifications
You must be signed in to change notification settings - Fork 55
fix(Alert): add isFromKeyboard
to state
#1238
Conversation
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.
|
@@ -108,6 +130,7 @@ class Alert extends UIComponent<ReactProps<AlertProps>> { | |||
return ( | |||
<ElementType | |||
className={classes.root} | |||
onFocus={this.handleFocus} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be added on the action, instead of the root element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Alert itself as far as I understand is not focusable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current points:
- as I see the whole
Alert
is not focusable by spec, so currently onlyaction
slot can be focused focus
event will be propogated from theaction
slot
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay! Thanks so much for fixing this! :)
Fixes #1230.