-
-
Notifications
You must be signed in to change notification settings - Fork 157
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
fix: refactor js code #39
Conversation
✅ Deploy Preview for hugo-blog-awesome ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
This PR exceeds the recommended size of 200 lines. Kindly ensure that you are NOT addressing multiple issues in one PR. We recommend you to create atomic PRs which focus on a single change. |
This PR exceeds the recommended size of 200 lines. Kindly ensure that you are NOT addressing multiple issues in one PR. We recommend you to create atomic PRs which focus on a single change. |
This PR exceeds the recommended size of 200 lines. Kindly ensure that you are NOT addressing multiple issues in one PR. We recommend you to create atomic PRs which focus on a single change. |
this.checked | ||
? area.classList.add("blurry") | ||
: area.classList.remove("blurry"); | ||
}); |
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.
This can be replaced with CSS.
Unfortunately :has()
is still experimental and disabled by default in firefox
and unavailable in firefox android
.
See: MDN and caniuse
Also there is a polyfill
So I'm not sure is it a good idea to use it.
@hugo-sid what do you think about that?
// wrap with @media where #menu-trigger becomes visible
body:has(#menu-trigger:checked) {
.wrapper {
animation: 0.2s ease-in forwards blur;
-webkit-animation: 0.2s ease-in forwards blur;
}
}
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.
@kusyka911, first thanks for this PR.
Note 1
Here is what I have to say regarding the use of CSS :has()
:
Thanks for the caniuse link. Looks like the global support for this property is only 86%.
As a safety measure, I have set the following bars for myself:
- global support > 95% - good to go
- global support >= 90% - we can consider using this
- global support <90 - we should not consider using this
Note 2
On the other hand, global support for JS classList.add
is 99.1%. So, I feel this is surely a better way to implement this functionality.
Note 3
INMHO, I don't think we should use the polyfill either. For this particular theme, I have decided to follow a minimal approach. Loading an external library for something which can be done with few lines of vanilla JS is not a good idea I guess.
Conclusion
In conclusion, I feel we should not use CSS :has()
. It's not worth sacrificing browser support for the few lines of JS.
What do you say @kusyka911 ?
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.
@hugo-sid thanks for your response.
In general I agree, but please also do not forget about no-script environments 😉
We can add polyfill but in this case it will also not work in no-script environments.
So I don't see any good reason to push this idea.
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.
We can add polyfill but in this case it will also not work in no-script environments.
Nice observation 😄
do not forget about no-script environments
I don't think we can do much for these.
This PR exceeds the recommended size of 200 lines. Kindly ensure that you are NOT addressing multiple issues in one PR. We recommend you to create atomic PRs which focus on a single change. |
(cherry picked from commit 5163fd1532995d81b22bbcc908629b28111a9e71)
This PR exceeds the recommended size of 200 lines. Kindly ensure that you are NOT addressing multiple issues in one PR. We recommend you to create atomic PRs which focus on a single change. |
This PR exceeds the recommended size of 200 lines. Kindly ensure that you are NOT addressing multiple issues in one PR. We recommend you to create atomic PRs which focus on a single change. |
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.
@kusyka911, I have created a new commit which does a minor refactoring of the docs in config.toml
.
I would love to hear your insight on the question relating to | fingerprint
.
layouts/partials/scriptsBodyEnd.html
Outdated
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.
This is a great idea @kusyka911 .
I feel, bundling JS to reduce number of network requests is crucial for improving website performance, as each request adds latency.
{{ $theme_script := resources.Get "js/theme.js" | minify | fingerprint }} | ||
<script src="{{ $theme_script.RelPermalink }}" integrity="{{ $theme_script.Data.Integrity }}"></script> | ||
{{ else }} | ||
{{ $theme_script := resources.Get "js/theme.js" | fingerprint }} |
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.
@kusyka911, do you think | fingerprint
is useful, even when we are not using the integrity
attribute in the script tag?
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.
@hugo-sid
Fingerprint will be added to filename. This should prevent JS caching by browser while you running site in development mode and making JS changes. At least this is why I prefer to add it.
P.S. I'm sorry for late response, right now I'm a little busy working on another small project.
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.
Thanks for your response @kusyka911 .
This should prevent JS caching by browser
I have another perspective though:
- If someone is concerned about caching, most likely they will disable it by going it to Dev tools. This is because they are concerned not only about JS caching but everything else (CSS, other assets) as well. The computation involved in calculating hash (due to
| fingerprint
) is rendered useless in this case.
Moreover, the wastage of computation can be quite high in development environment since, file changes (and thus hash computation) can happen very frequently. - If someone is not concerned about caching, the computation involved in calculating hash is anyways not going to be useful. Serving only fresh JS may not help much.
Thus, I feel it's not a good idea to have fingerprint
in development. Let me know if you have any other thoughts in this regard.
P.S. I'm sorry for late response, right now I'm a little busy working on another small project.
No worries. That's aright.
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.
I have proceeded with removing fingerprint
in development. However, please feel free to submit a PR, if you see any good use case.
This PR exceeds the recommended size of 200 lines. Kindly ensure that you are NOT addressing multiple issues in one PR. We recommend you to create atomic PRs which focus on a single change. |
This PR exceeds the recommended size of 200 lines. Kindly ensure that you are NOT addressing multiple issues in one PR. We recommend you to create atomic PRs which focus on a single change. |
Kudos, SonarCloud Quality Gate passed! |
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.
Thanks @kusyka911 for refactoring JS.
I appreciate your contribution.
some refactoring and optimization for JS code
What problem does this PR solve?
main.js
script what will be concatenated with other scripts.goToTop.js
toload
event handler.scriptBodyEnd.html
to add scripts in more easy way.Is this PR adding a new feature?
Add ability to add custom scripts to the site via configuration
Is this PR related to any issue or discussion?
no
PR Checklist