Skip to content

Adding opts.attrs and opts.text.#9

Merged
defunctzombie merged 1 commit intoeldargab:masterfrom
jsdevel:adding-opts
Mar 6, 2015
Merged

Adding opts.attrs and opts.text.#9
defunctzombie merged 1 commit intoeldargab:masterfrom
jsdevel:adding-opts

Conversation

@jsdevel
Copy link
Copy Markdown
Contributor

@jsdevel jsdevel commented Mar 2, 2015

Some of the 3rd party libraries my team mates have been dealing with require text nodes in the script tag and / or attributes. This PR adds support for this.

@joshuajohnson814
Copy link
Copy Markdown

+1

1 similar comment
@tylerkayser
Copy link
Copy Markdown

+1

@vvo
Copy link
Copy Markdown

vvo commented Mar 2, 2015

We could also just return the script DOM node, what you do with it is then up to you. Add text node, attributes.

@vvo
Copy link
Copy Markdown

vvo commented Mar 2, 2015

This would not add features for a small need.

@jsdevel
Copy link
Copy Markdown
Contributor Author

jsdevel commented Mar 2, 2015

@vvo by then it's too late as the libs we're calling sometimes require attributes on the script tag that loaded them while the js is executing E.G. before onload has fired.

@vvo
Copy link
Copy Markdown

vvo commented Mar 2, 2015

But creating a script tag is synchronous compared to doing the actual script request. So if you add attributes right after using loadScript which returns the dom element, it should always work right?

@vvo
Copy link
Copy Markdown

vvo commented Mar 2, 2015

Maybe if there's a cached version you are screwed, dunno.

@jsdevel
Copy link
Copy Markdown
Contributor Author

jsdevel commented Mar 2, 2015

I can see cases where {async: false} may be needed so the third parameter doesn't seem too far off to me.

@defunctzombie
Copy link
Copy Markdown
Collaborator

If there is no way to hack around this then we should have the options.

Separately, the PR needs to update the README with new features.

@jsdevel jsdevel force-pushed the adding-opts branch 2 times, most recently from 8fb4b2f to d8c5af3 Compare March 3, 2015 05:04
@jsdevel
Copy link
Copy Markdown
Contributor Author

jsdevel commented Mar 3, 2015

@defunctzombie Readme.md updated. Let me know if you'd like the format changed.

Also, if there is a way to hack around this then I'm completely open to it.

@vvo
Copy link
Copy Markdown

vvo commented Mar 3, 2015

@jsdevel As for the async atribute, you are right, let's add this feature :)

@jsdevel
Copy link
Copy Markdown
Contributor Author

jsdevel commented Mar 3, 2015

@vvo tests added for opts.async, opts.charset, and opts.type.

@jsdevel jsdevel mentioned this pull request Mar 3, 2015
@jsdevel
Copy link
Copy Markdown
Contributor Author

jsdevel commented Mar 5, 2015

@vvo @defunctzombie are there any other tweaks you'd like me to make?

@defunctzombie
Copy link
Copy Markdown
Collaborator

LGTM

@vvo
Copy link
Copy Markdown

vvo commented Mar 6, 2015

👍 But I am no maintainer :)

defunctzombie added a commit that referenced this pull request Mar 6, 2015
Adding opts.attrs and opts.text.
@defunctzombie defunctzombie merged commit c8a2155 into eldargab:master Mar 6, 2015
@defunctzombie
Copy link
Copy Markdown
Collaborator

@jsdevel

<internet explorer 6 on Windows 2003> opts.text
Error: Unexpected call to method or property access.

Tests fail on old IE (6,7,8).

Please take a look and fix this up otherwise will have to revert since my goal with this is to have it work on all browsers.

@vvo
Copy link
Copy Markdown

vvo commented Mar 6, 2015

@jsdevel either setAttribute or createTextNode are failing I guess.

@jsdevel
Copy link
Copy Markdown
Contributor Author

jsdevel commented Mar 6, 2015

Looking now...

@jsdevel
Copy link
Copy Markdown
Contributor Author

jsdevel commented Mar 6, 2015

Firing up my vista VM to test in IE8...

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.

5 participants