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.

Sign up for GitHub

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

Jump to bottom

Update js deps and move to unveil on cdn #2893

Merged
merged 5 commits into from
Mar 25, 2018
Merged

Update js deps and move to unveil on cdn #2893

merged 5 commits into from
Mar 25, 2018

Conversation

stephengroat
Copy link
Contributor

@stephengroat stephengroat commented Dec 15, 2017 •
edited
Loading

move to unveil2 on cdnjs, update all other deps

functionality verified, should be reviewed again

mxxcon
mxxcon reviewed Dec 15, 2017
View reviewed changes
Copy link
Contributor

@mxxcon mxxcon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

perhaps use jsdelivr for all scripts to minimize time spent on looking up and connecting to various cdns?

<script  src="https://cdn.jsdelivr.net/npm/jquery@3.2.1/dist/jquery.min.js" integrity="sha256-hwg4gsxgFZhOsEEamdOYGBf13FyQuiTwlAQgxVSNgt4=" crossorigin="anonymous"></script>
<script  src="https://cdn.jsdelivr.net/npm/semantic-ui@2.2.13/dist/semantic.min.js" integrity="sha256-Bhi6GMQ/72uYZcJXCJ2LToOIcN3+Cx47AZnq/Bw1f7A=" crossorigin="anonymous"></script>
<script  src="https://cdn.jsdelivr.net/npm/unveil2@2.0.8/src/jquery.unveil2.js" integrity="sha256-TJeKT7YYibjK/MKACXmyJbZzKgIASoiFZqYrySyh1cQ=" crossorigin="anonymous"></script>
<script  src="https://cdn.jsdelivr.net/npm/jets@0.14.0/jets.min.js" integrity="sha256-0wXdZ7VL+Nng2jfkSxgChagPiF6ZzDToGwzomq/AfBw=" crossorigin="anonymous"></script>

@stephengroat
Copy link
Contributor Author

@mxxcon all libs now available on cdnjs

@mxxcon
Copy link
Contributor

mxxcon commented Dec 18, 2017

You probably wanted to use minified version?

<script src="https://cdnjs.cloudflare.com/ajax/libs/unveil2/2.0.8/jquery.unveil2.min.js" integrity="sha256-B00tEEtJRbA9gas0viRdqVPI81EuZG+kYU978/alKt8=" crossorigin="anonymous"></script>

@stephengroat stephengroat added the enhancement Issue/PR contains enhancements to the overall code of the site. label Feb 7, 2018
@stephengroat stephengroat mentioned this pull request Feb 24, 2018
Merged
@fpigerre fpigerre merged commit 8e4a0fa into 2factorauth:master Mar 25, 2018
@stephengroat
Copy link
Contributor Author

@PSGS i'm now seeing superbad jquery times on page processing (didn't see this before, might be something new in chrome since this came out). are you seeing the same thing? might want to revert if this is the case

screen shot 2018-03-26 at 10 05 22 am

@RichJeanes
Copy link
Member

RichJeanes commented Mar 26, 2018 •
edited
Loading

What did you do on the page to get that performance? I'm browsing and searching the page without issue in Firefox 59.0 32bit.

@stephengroat
Copy link
Contributor Author

this is when it's locally loading from bundle install && bundle exec jekyll serve with these new merges.

i think it's not liking my local serving now (something with new jekyll or something else that i have no idea about), i just want to make sure that it's an issue confined to my local system and not something more systemic that we need to revert

@Carlgo11
Copy link
Member

Same for me
screenshot

@stephengroat stephengroat deleted the update-js branch April 5, 2018 04:16
@stephengroat
Copy link
Contributor Author

fixed by #3090

@fpigerre
Copy link
Contributor

fpigerre commented Apr 5, 2018 •
edited
Loading

Hello,

It looks like the jQuery Unveil plugin is running some sort of recursive QuickSort algorithm that's chewing up resources. As the Unveil plugin is no longer maintained and doesn't seem to be versioned very well, I would suggest moving to jQuery's lazy loading library (proposed in #3090). I can confirm this issue does not present itself as of commit a917e41.

screen shot 2018-04-05 at 14 02 48

Thanks,
psgs 🌴

@fpigerre fpigerre mentioned this pull request Apr 5, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpigerre fpigerre fpigerre approved these changes

@mxxcon mxxcon mxxcon approved these changes

Assignees
No one assigned
Labels
enhancement Issue/PR contains enhancements to the overall code of the site.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@stephengroat @mxxcon @RichJeanes @Carlgo11 @fpigerre