Skip to content

Conversation

scherler
Copy link
Collaborator

@scherler scherler commented Oct 13, 2017

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

  • Link to JIRA ticket in description, if appropriate.
  • Change is code complete and matches issue description
  • Appropriate unit or acceptance tests or explanation to why this change has no tests
  • Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

  • Run the changes and verified the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

@michaelneale
Copy link
Member

ping @sophistifunk @NicuPascu can one of you review?

Copy link
Collaborator

@NicuPascu NicuPascu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sophistifunk
Copy link
Collaborator

LGTM. Wasn't this already done earlier? Or should I lay off the shrooms?

@michaelneale
Copy link
Member

@sophistifunk

@@ -66,6 +70,7 @@ class SvgIcon extends Component {
return (
<svg
{...other}
focusable={focusable}
Copy link
Collaborator

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)

Copy link
Collaborator

@kzantow kzantow left a 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

@scherler
Copy link
Collaborator Author

06a1470#diff-9d44d2134191673503efac16ed92b580 @sophistifunk yeah lots of places and dropped the prop

Copy link
Contributor

@cliffmeyers cliffmeyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 🐝

@cliffmeyers cliffmeyers dismissed kzantow’s stale review October 23, 2017 14:20

Requested changes were implemented

@scherler scherler changed the title Ie11 fixes [JENKINS-47491] Ie11 fixes Oct 24, 2017
@scherler scherler merged commit c405f7d into master Oct 24, 2017
@scherler scherler deleted the IE11Fixes branch October 24, 2017 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants