Skip to content
This repository has been archived by the owner on Sep 10, 2020. It is now read-only.

[Website] Added "Copy Script Tag with SRI" option to library search results #251

Merged
merged 1 commit into from
May 5, 2019

Conversation

Glennmen
Copy link
Contributor

@Glennmen Glennmen commented Oct 6, 2018

I do have experience in JS but this was the first time I worked with a Node.js project. I would perfectly understand if I made any mistakes.

So I have actually "reported" about this almost a year ago on Twitter and @PeterDaveHello challenged me to make a PR for it. I have never forgotten about it and now because of Hacktoberfest I decided to make some time for it and try it out.

I also saw some open issues about the same issue:
Fixes #152 SRI support on search result
Fixes cdnjs/cdnjs#12746 Add Copy Script Tag with SRI option

@ddadon10
Copy link

Hello everyone,

I've just read https://news.ycombinator.com/item?id=18559786

TL;DR: Major sites has JavaScript tag without integrity attribute on their payment pages.

It would be very useful to see this PR merged,

Dropdown menu on the search page https://cdnjs.com/
screenshot 2018-11-29 at 17 59 03

Dropdown menu on the library page https://cdnjs.com/libraries/jquery
screenshot 2018-11-29 at 17 59 23

Thanks a lot

@MattIPv4 MattIPv4 mentioned this pull request Jan 1, 2019
MattIPv4
MattIPv4 previously approved these changes Jan 2, 2019
Copy link
Member

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

LGTM

@MattIPv4 MattIPv4 changed the title Added "Copy Script Tag with SRI" option to library search results [Website] Added "Copy Script Tag with SRI" option to library search results Jan 2, 2019
@MattIPv4
Copy link
Member

@PeterDaveHello Could we get this merged?

@Glennmen
Copy link
Contributor Author

@PeterDaveHello Any update please.

@Glennmen
Copy link
Contributor Author

Glennmen commented May 1, 2019

@PeterDaveHello Can you please review this PR, let me know if I need to change anything.

@PeterDaveHello
Copy link
Contributor

@Glennmen I'll try to review that ASAP, sorry for the delay.

@MattIPv4
Copy link
Member

MattIPv4 commented May 1, 2019

@PeterDaveHello Is it worth manually tidying the commits here, or are you good to do a squash merge as this can all be one commit?

Copy link
Contributor

@PeterDaveHello PeterDaveHello left a comment

Choose a reason for hiding this comment

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

Let's try it!!

@MattIPv4
Copy link
Member

MattIPv4 commented May 5, 2019

@PeterDaveHello Merge?

@PeterDaveHello
Copy link
Contributor

@MattIPv4 Wait a sec, I am now tring to verify the result 😉

PeterDaveHello
PeterDaveHello previously approved these changes May 5, 2019
Copy link
Contributor

@PeterDaveHello PeterDaveHello left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@PeterDaveHello PeterDaveHello left a comment

Choose a reason for hiding this comment

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

Let's merge it now.

@PeterDaveHello PeterDaveHello merged commit a4c2ff8 into cdnjs:master May 5, 2019
@Glennmen
Copy link
Contributor Author

Glennmen commented May 5, 2019

@PeterDaveHello & @MattIPv4 Thanks for reviewing and finally getting it merged ❤️

@PeterDaveHello
Copy link
Contributor

@Glennmen Thank you for your contribution 😆

@MattIPv4
Copy link
Member

MattIPv4 commented May 5, 2019

@Glennmen Incredibly useful contribution that I am looking forward to making use of personally, so thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Copy Script Tag with SRI option SRI support on search result
4 participants