search

jquery / codeorigin.jquery.com

jQuery CDN
https://releases.jquery.com
Other
57 stars 602 forks source link

Adding SRI support to codeorigin.jquery.com #22

Closed jmervine closed 8 years ago

jmervine commented 8 years ago

Hello!

Per the discussions on #20, I'm adding SRI generation and include the class and data-hash attributes here in jquery/codeorigin.jquery.com. The changes here should have no functional impact on the site, but are required for implementation. I'm new to both the *.jquery.com setup and Wordpress itself, however based on what I'm seeing, it looks like the rest will need to be implemented in github.com/jquery/jquery-wp-content inside themes/codeorigin.jquery.com. If I am incorrect in that assumption, please let me know.

Additionally, a question for the group. I opted to include the generated sri-directives.json file, but still generate it on deployment. I wanted to get some feedback on this. I sort of think it should be one or the other. Either (a) generate on deployment (adding it to the .gitignore file) or (b) required it to be manually generated and checked in. Thoughts?

Thanks and cheers! J

(cc @jdorfman)

jquerybot commented 8 years ago

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

:memo: Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.

If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

jmervine commented 8 years ago

Point of note. I signed the CLA (twice) and verified that both the name and email address that I used match what's in my github.com settings.

scottgonzalez commented 8 years ago

There are a lot of code style violations with spacing, but we can fix that when merging.

scottgonzalez commented 8 years ago

it looks like the rest will need to be implemented in github.com/jquery/jquery-wp-content inside themes/codeorigin.jquery.com. If I am incorrect in that assumption, please let me know.

That's correct. If you have any other questions about the way the sites work or if you run into any issues getting everything set up, just let us know.

Additionally, a question for the group. I opted to include the generated sri-directives.json file, but still generate it on deployment.

Just add it to .gitignore. Alternatively, place the generated file in dist/resources/ and it will not only be ignored, but will be published to the site for others to use if they so desire (it would be available at https://code.jquery.com/resources/sri-directives.json).

scottgonzalez commented 8 years ago

Point of note. I signed the CLA (twice) and verified that both the name and email address that I used match what's in my github.com settings.

Indeed. However, the CLA check doesn't look at GitHub, it looks at the actual commits in git. In git, you have your name set as Josh, not Joshua. You can either update your git config and reset the author info on these commits, or sign the CLA as Josh.

On a related note, I see that the CLA status page is 404ing. I'll try to take a look at that tomorrow.

jmervine commented 8 years ago

@scottgonzalez Thanks for the feedback. I'll address everything mentioned. If there's a published style guide, or linter that you use, I'd be happy to address any violations.

Cheers and thanks again! J

scottgonzalez commented 8 years ago

If there's a published style guide, or linter that you use, I'd be happy to address any violations.

http://contribute.jquery.org/style-guide/js/

We generally lint code with JSHint and JSCS, but we don't currently run those on our site repos.

jmervine commented 8 years ago

Cool. I'll review the style guide tonight and do my best to fix what's missing. I think I got all the spacing specific issues though.