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

Legacy Cleanup #8

Merged
merged 11 commits into from
Jan 11, 2017
Merged

Legacy Cleanup #8

merged 11 commits into from
Jan 11, 2017

Conversation

noslouch
Copy link
Contributor

Updates for better legacy compat. Depends on nypublicradio/publisher#6.

@noslouch noslouch force-pushed the brian-legacy-cleanup branch 4 times, most recently from be67056 to dcddf30 Compare January 10, 2017 23:40
@@ -2,5 +2,8 @@ export default function isJavascript(scriptTag) {
// TODO: add a console warning if the script tag doesn't have an attribute?
// seems like it's required for some parts of ember consumption
let type = scriptTag.attributes.type ? scriptTag.attributes.type.value : 'text/javascript';
return /(?:application|text)\/(deferred-)?javascript/i.test(type);
// guard against chrome-extension:// scripts
let badSrc = scriptTag.src && !scriptTag.src.match(/^http|^\//);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking on this, there's no worry of a relative import being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what you mean here

Copy link
Contributor

@mike-hearn mike-hearn Jan 11, 2017

Choose a reason for hiding this comment

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

I was just wondering if there would ever be a case where src would be relative to the HTML it's being used in, e.g. <script src="./bundle.js">, which would not match either ^http or ^/, but would still potentially be valid JS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, yeah. THAT IS A GOOD CATCH.

Copy link
Contributor

Choose a reason for hiding this comment

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

I DID IT

{{link-to 'All Streams' 'stream' class="inherit" preventDefault=false}}
{{/link-to}}

{{#link-to 'schedule' current-when="schedule.date" tagName='li' href="false" class="list-item list-item--nav" }}
{{#link-to 'schedule' current-when="schedule.date" tagName='li' href=false class="list-item list-item--nav"}}
Copy link
Contributor

@mike-hearn mike-hearn Jan 11, 2017

Choose a reason for hiding this comment

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

Minor quibble slash question for my own future coding: do we have a rule in our codebase for when to use singe vs. double quotes? It looks inconsistent in this file, for example current-when on this line vs. L124.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's no rule, but I prefer single quotes in all cases and escaping apostrophes in longer strings, but that's me.

Copy link
Contributor

@mike-hearn mike-hearn left a comment

Choose a reason for hiding this comment

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

Made a couple of minor comments but otherwise... LGTM.

@noslouch noslouch merged commit 0def215 into master Jan 11, 2017
@noslouch noslouch deleted the brian-legacy-cleanup branch January 11, 2017 21:47
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.

2 participants