-
-
Notifications
You must be signed in to change notification settings - Fork 7
[BugFix] Discord embed, resume views, and scripts loading issues #27
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
[BugFix] Discord embed, resume views, and scripts loading issues #27
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
time to do some investigation, i guess
<script src="assets/js/jquery-3.6.1.min.js" async></script> | ||
<script src="assets/js/jquery.scrollex.min.js" async></script> | ||
<script src="assets/js/jquery.scrolly.min.js" async></script> | ||
<script src="assets/js/browser.min.js" async></script> | ||
<script src="assets/js/breakpoints.min.js" async></script> | ||
<script src="assets/js/util.js" async></script> | ||
<script src="assets/js/main.js" async></script> | ||
<script src="https://cdn.jsdelivr.net/npm/@widgetbot/html-embed@1.0.0"></script> | ||
<script src="assets/js/jquery-3.6.1.min.js"></script> | ||
<script src="assets/js/jquery.scrollex.min.js" ></script> | ||
<script src="assets/js/jquery.scrolly.min.js"></script> | ||
<script src="assets/js/browser.min.js" ></script> | ||
<script src="assets/js/breakpoints.min.js" ></script> | ||
<script src="assets/js/util.js" ></script> | ||
<script src="assets/js/main.js" ></script> | ||
<script src="https://cdn.jsdelivr.net/npm/@widgetbot/html-embed@1.3.0"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while this is probably a sane choice, its likely important to figure out why async
was added in the first place.
i would not be surprised if it was done to cut down response time for the website which in turn increases SEO massively.
so if thats the case, just undoing this might not be a good choice given that the websites primary role is to be SE-relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was done in the big SEO PR: #10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since develop
doesnt have it to begin with, merging this will hence not make the SEO worse. So we can accept this change for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested locally, looks good. thanks
Found 3 issues:
closes #20