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

check for typeof attribute value, if is string use setAttribute #511

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@enapupe

enapupe commented Jan 17, 2017

I wasn't able to run react tests against this change but I tried to change the logic as minimum as possible.

ref #492

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 17, 2017

Coverage Status

Coverage increased (+0.008%) to 97.715% when pulling 69bd4bd on enapupe:dom-setacessor-setAttribute-if-string into 99ad1f3 on developit:master.

coveralls commented Jan 17, 2017

Coverage Status

Coverage increased (+0.008%) to 97.715% when pulling 69bd4bd on enapupe:dom-setacessor-setAttribute-if-string into 99ad1f3 on developit:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 17, 2017

Coverage Status

Coverage increased (+0.008%) to 97.715% when pulling 69bd4bd on enapupe:dom-setacessor-setAttribute-if-string into 99ad1f3 on developit:master.

coveralls commented Jan 17, 2017

Coverage Status

Coverage increased (+0.008%) to 97.715% when pulling 69bd4bd on enapupe:dom-setacessor-setAttribute-if-string into 99ad1f3 on developit:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 17, 2017

Coverage Status

Coverage increased (+0.008%) to 97.715% when pulling 69bd4bd on enapupe:dom-setacessor-setAttribute-if-string into 99ad1f3 on developit:master.

coveralls commented Jan 17, 2017

Coverage Status

Coverage increased (+0.008%) to 97.715% when pulling 69bd4bd on enapupe:dom-setacessor-setAttribute-if-string into 99ad1f3 on developit:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 17, 2017

Coverage Status

Coverage remained the same at 97.707% when pulling d884a2a on enapupe:dom-setacessor-setAttribute-if-string into 99ad1f3 on developit:master.

coveralls commented Jan 17, 2017

Coverage Status

Coverage remained the same at 97.707% when pulling d884a2a on enapupe:dom-setacessor-setAttribute-if-string into 99ad1f3 on developit:master.

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Jan 17, 2017

Owner

Looking at the travis build, it actually seems like this might be better for performance! (not sure how but I'll take it)

Owner

developit commented Jan 17, 2017

Looking at the travis build, it actually seems like this might be better for performance! (not sure how but I'll take it)

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 18, 2017

Coverage Status

Coverage remained the same at 97.707% when pulling a5c83c4 on enapupe:dom-setacessor-setAttribute-if-string into dfa5912 on developit:master.

coveralls commented Jan 18, 2017

Coverage Status

Coverage remained the same at 97.707% when pulling a5c83c4 on enapupe:dom-setacessor-setAttribute-if-string into dfa5912 on developit:master.

@enapupe

This comment has been minimized.

Show comment
Hide comment
@enapupe

enapupe Jan 18, 2017

Don't forget to consider merging #495 after fixing this one.

enapupe commented Jan 18, 2017

Don't forget to consider merging #495 after fixing this one.

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Jan 18, 2017

Owner

@enapupe yup yup, I'll pair up the two. These are likely to go into 8.0 as the behavior probably constitutes a breaking change, or at least it has a chance to be "breaking".

Owner

developit commented Jan 18, 2017

@enapupe yup yup, I'll pair up the two. These are likely to go into 8.0 as the behavior probably constitutes a breaking change, or at least it has a chance to be "breaking".

@robdodson

This comment has been minimized.

Show comment
Hide comment
@robdodson

robdodson Jul 16, 2017

I was chatting with @developit about this PR today and he suggested adding a check to see if the value being set is an object and if so, falling back to using a property setter. I tried to outline the expected behavior in this comment (sorry it's a bit long!)

robdodson commented Jul 16, 2017

I was chatting with @developit about this PR today and he suggested adding a check to see if the value being set is an object and if so, falling back to using a property setter. I tried to outline the expected behavior in this comment (sorry it's a bit long!)

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