Added a setter for class attr to handle svg elements where className cann… #2048

Merged
merged 2 commits into from Nov 6, 2015

Conversation

Projects
None yet
2 participants
@Alfredo-Delgado
Contributor

Alfredo-Delgado commented Oct 30, 2015

…ot be written to like svg. Resolves #2015

@Alfredo-Delgado Alfredo-Delgado added this to the 2.3.2 milestone Oct 30, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 31, 2015

Contributor

This shouldn't be reading className. I think it should just detect if the element is an svg element and set the attribute.

Sent from my iPhone

On Oct 30, 2015, at 5:31 PM, Alfredo Delgado notifications@github.com wrote:

…ot be written to like svg. Resolves #2015

You can view, comment on, or merge this pull request online at:

#2048

Commit Summary

Added a setter for class attr to handle elements where className cannot be written to like svg. Resolves #2015
File Changes

M util/attr/attr.js (14)
M util/attr/attr_test.js (20)
Patch Links:

https://github.com/canjs/canjs/pull/2048.patch
https://github.com/canjs/canjs/pull/2048.diff

Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Oct 31, 2015

This shouldn't be reading className. I think it should just detect if the element is an svg element and set the attribute.

Sent from my iPhone

On Oct 30, 2015, at 5:31 PM, Alfredo Delgado notifications@github.com wrote:

…ot be written to like svg. Resolves #2015

You can view, comment on, or merge this pull request online at:

#2048

Commit Summary

Added a setter for class attr to handle elements where className cannot be written to like svg. Resolves #2015
File Changes

M util/attr/attr.js (14)
M util/attr/attr_test.js (20)
Patch Links:

https://github.com/canjs/canjs/pull/2048.patch
https://github.com/canjs/canjs/pull/2048.diff

Reply to this email directly or view it on GitHub.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 31, 2015

Contributor

Also, I think if someone set a className to {toString: function(){ return "something"}}, this would fail.

Contributor

justinbmeyer commented Oct 31, 2015

Also, I think if someone set a className to {toString: function(){ return "something"}}, this would fail.

@Alfredo-Delgado

This comment has been minimized.

Show comment
Hide comment
@Alfredo-Delgado

Alfredo-Delgado Oct 31, 2015

Contributor
  • I added tests for {toString: function(){ return "something"}} and they pass.
  • The criterium for using setAttribute was updated to only apply to SVGElement instances.
Contributor

Alfredo-Delgado commented Oct 31, 2015

  • I added tests for {toString: function(){ return "something"}} and they pass.
  • The criterium for using setAttribute was updated to only apply to SVGElement instances.

Alfredo-Delgado added a commit that referenced this pull request Oct 31, 2015

@Alfredo-Delgado Alfredo-Delgado changed the title from Added a setter for class attr to handle elements where className cann… to Added a setter for class attr to handle svg elements where className cann… Oct 31, 2015

Alfredo-Delgado added a commit that referenced this pull request Oct 31, 2015

Alfredo-Delgado added a commit that referenced this pull request Oct 31, 2015

Alfredo-Delgado added a commit that referenced this pull request Oct 31, 2015

util/attr/attr.js
+ "class": function(el, val) {
+ val = val || '';
+
+ if(el instanceof SVGElement) {

This comment has been minimized.

@Alfredo-Delgado

Alfredo-Delgado Nov 1, 2015

Contributor

Thinking of IE8, this won't work. I'll update to if(el.namespaceURI === 'http://www.w3.org/2000/svg') like modernizr.

@Alfredo-Delgado

Alfredo-Delgado Nov 1, 2015

Contributor

Thinking of IE8, this won't work. I'll update to if(el.namespaceURI === 'http://www.w3.org/2000/svg') like modernizr.

This comment has been minimized.

justinbmeyer added a commit that referenced this pull request Nov 6, 2015

Merge pull request #2048 from canjs/class-2015
Added a setter for class attr to handle svg elements where className cann…

@justinbmeyer justinbmeyer merged commit ae99674 into master Nov 6, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@justinbmeyer justinbmeyer deleted the class-2015 branch Nov 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment