Skip to content
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

Update html.js (#18703) #152

Closed
wants to merge 4 commits into from
Closed

Update html.js (#18703) #152

wants to merge 4 commits into from

Conversation

willgriffin
Copy link
Contributor

currently if a number is passed as the second argument to the dojo.html.set method it results in a "Failed to execute 'appendChild' on 'Node': parameter 1 is not of type 'Node'" error and as i result i no longer have any hair left as i've ripped it all out. If javascript is automatically going to determine the variable type it stands to reason that numbers should be synonymous with strings in this instance

currently if a number is passed as the second argument to the dojo.html.set method it results in a "Failed to execute 'appendChild' on 'Node': parameter 1 is not of type 'Node'" error and as i result i no longer have any hair left as i've ripped it all out. If javascript is automatically going to determine the variable types it stands to reason that numbers should be synonymous with strings in this instance
@wkeese
Copy link
Member

wkeese commented May 2, 2015

I'm conceptually OK with that change but the two objections I have are:

  1. There are many methods in that file that (like set()) are documented to take a string but not a number. Why just fix set()?
  2. The fix should have a unit test.

@willgriffin
Copy link
Contributor Author

Is that better? I wasn't sure whether i should add Number as a allowable type so i just left it as is.

@wkeese
Copy link
Member

wkeese commented May 3, 2015

Seems reasonable to me.

@dylans
Copy link
Member

dylans commented Jul 12, 2015

I'm fine with this PR, but I don't see a CLA on file for @willgriffin . Please see https://github.com/dojo/dojo/blob/master/CONTRIBUTING.md and http://dojofoundation.org/about/claForm in particular, and then we can land this patch. Unless this is covered under a company CLA, and in that case, just let us know.

@dylans
Copy link
Member

dylans commented Sep 11, 2015

@willgriffin please see my previous note about having a CLA on file.

@dylans dylans added this to the 1.11 milestone Sep 11, 2015
@dylans dylans changed the title Update html.js Update html.js (#18703) Sep 11, 2015
@willgriffin
Copy link
Contributor Author

Hey, Sorry about missing your message the first time around.. I've
completed the CLA now.

On Fri, Sep 11, 2015 at 8:19 AM, Dylan Schiemann notifications@github.com
wrote:

@willgriffin https://github.com/willgriffin please see my previous note
about having a CLA on file.


Reply to this email directly or view it on GitHub
#152 (comment).

@dylans
Copy link
Member

dylans commented Sep 12, 2015

Closed via 1b18e24. Given that it is a subtle change in behavior, I'm not going to backport unless people think we should. Thanks for your first commit to Dojo @willgriffin !

@dylans dylans closed this Sep 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants