-
Notifications
You must be signed in to change notification settings - Fork 537
[JENKINS-47491] Ie11 fixes #1490
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.
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
Conversation
ping @sophistifunk @NicuPascu can one of you review? |
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.
LGTM
LGTM. Wasn't this already done earlier? Or should I lay off the shrooms? |
@@ -66,6 +70,7 @@ class SvgIcon extends Component { | |||
return ( | |||
<svg | |||
{...other} | |||
focusable={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.
Why not just drop the option and have focusable={false}
? ... but wasn't this change already done? (as @sophistifunk said)
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.
no need for an option, should never be focusable, can wrap in a focusable element with text that makes sense like a button if need be
06a1470#diff-9d44d2134191673503efac16ed92b580 @sophistifunk yeah lots of places and dropped the prop |
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.
Looks good! 🐝
Requested changes were implemented
Description
https://issues.jenkins-ci.org/browse/JENKINS-47491
PolymerElements/iron-iconset-svg#46
Any SVG elements written by SvgIcon will be put into the tab order of the page in IE11.
Expected outcome
SVG elements should not be focusable by default
Submitter checklist
Reviewer checklist