fix: refactor js code #39

kusyka911 · 2023-05-05T14:54:38Z

some refactoring and optimization for JS code

What problem does this PR solve?

Make single JS for theme initialization and switching. I think it's not so big and it is ok. This also prevent exposing data and functions to global scope.
Create empty main.js script what will be concatenated with other scripts.
Wrap goToTop.js to load event handler.
Optimize code of scriptBodyEnd.html to add scripts in more easy way.
ability to add custom scripts to the site via configuration

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

I have verified that the code works as described/as intended.
This change adds a social icon which has a permissive license to use it.
This change does not include any external library/resources.
This change does not include any unrelated scripts (e.g. bash and python scripts).
I have enabled maintainer edits for this PR.

netlify · 2023-05-05T14:54:46Z

✅ Deploy Preview for hugo-blog-awesome ready!

Name	Link
🔨 Latest commit	`c32957e`
🔍 Latest deploy log	https://app.netlify.com/sites/hugo-blog-awesome/deploys/646a0573b620ce00083ac7ff
😎 Deploy Preview	https://deploy-preview-39--hugo-blog-awesome.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions · 2023-05-05T14:54:57Z

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.

github-actions · 2023-05-05T14:58:55Z

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.

github-actions · 2023-05-05T16:56:09Z

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.

kusyka911 · 2023-05-05T17:26:00Z

assets/js/theme.js

+            this.checked
+                ? area.classList.add("blurry")
+                : area.classList.remove("blurry");
+        });


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; } }

@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 ?

@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.

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.

github-actions · 2023-05-06T02:37:39Z

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)

github-actions · 2023-05-06T03:16:22Z

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.

github-actions · 2023-05-06T16:48:26Z

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.

hugo-sid

@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 .

hugo-sid · 2023-05-06T16:40:56Z

layouts/partials/scriptsBodyEnd.html

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.

hugo-sid · 2023-05-06T16:53:27Z

layouts/partials/scriptsBodyStart.html

+{{ $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 }}


@kusyka911, do you think | fingerprint is useful, even when we are not using the integrity attribute in the script tag?

@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.

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.

I have proceeded with removing fingerprint in development. However, please feel free to submit a PR, if you see any good use case.

github-actions · 2023-05-21T11:36:19Z

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.

github-actions · 2023-05-21T11:50:28Z

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.

sonarcloud · 2023-05-21T11:50:31Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

hugo-sid

Thanks @kusyka911 for refactoring JS.

I appreciate your contribution.

github-actions bot added the size/xl label May 5, 2023

kusyka911 force-pushed the main branch from cfb79d1 to 2a8ae35 Compare May 5, 2023 14:58

kusyka911 force-pushed the main branch from 2a8ae35 to 659cb68 Compare May 5, 2023 16:55

kusyka911 commented May 5, 2023

View reviewed changes

kusyka911 force-pushed the main branch from 659cb68 to 2b8f161 Compare May 6, 2023 02:37

fix: refactor js code

6ab4eaf

(cherry picked from commit 5163fd1532995d81b22bbcc908629b28111a9e71)

kusyka911 force-pushed the main branch from 2b8f161 to 6ab4eaf Compare May 6, 2023 03:16

docs: refactor docs regarding addition of scripts

12fa69a

hugo-sid reviewed May 6, 2023

View reviewed changes

kusyka911 requested a review from hugo-sid May 16, 2023 15:44

chore: remove | fingerprint in development environment

80f3f24

chore: remove | fingerprint in development environment (2)

c32957e

hugo-sid approved these changes May 21, 2023

View reviewed changes

hugo-sid merged commit 9d9f2bc into hugo-sid:main May 21, 2023

hugo-sid mentioned this pull request May 21, 2023

chore(main): release 1.2.2 #49

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: refactor js code #39

fix: refactor js code #39

kusyka911 commented May 5, 2023

netlify bot commented May 5, 2023 •

edited

Loading

github-actions bot commented May 5, 2023

github-actions bot commented May 5, 2023

github-actions bot commented May 5, 2023

kusyka911 May 5, 2023 •

edited

Loading

hugo-sid May 6, 2023 •

edited

Loading

kusyka911 May 6, 2023 •

edited

Loading

hugo-sid May 6, 2023 •

edited

Loading

github-actions bot commented May 6, 2023

github-actions bot commented May 6, 2023

github-actions bot commented May 6, 2023

hugo-sid left a comment •

edited

Loading

hugo-sid May 6, 2023 •

edited

Loading

hugo-sid May 6, 2023

kusyka911 May 16, 2023 •

edited

Loading

hugo-sid May 16, 2023 •

edited

Loading

hugo-sid May 21, 2023

github-actions bot commented May 21, 2023

github-actions bot commented May 21, 2023

sonarcloud bot commented May 21, 2023

hugo-sid left a comment

fix: refactor js code #39

fix: refactor js code #39

Conversation

kusyka911 commented May 5, 2023

some refactoring and optimization for JS code

What problem does this PR solve?

Is this PR adding a new feature?

Is this PR related to any issue or discussion?

PR Checklist

netlify bot commented May 5, 2023 • edited Loading

✅ Deploy Preview for hugo-blog-awesome ready!

github-actions bot commented May 5, 2023

github-actions bot commented May 5, 2023

github-actions bot commented May 5, 2023

kusyka911 May 5, 2023 • edited Loading

Choose a reason for hiding this comment

hugo-sid May 6, 2023 • edited Loading

Choose a reason for hiding this comment

Note 1

Note 2

Note 3

Conclusion

kusyka911 May 6, 2023 • edited Loading

Choose a reason for hiding this comment

hugo-sid May 6, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented May 6, 2023

github-actions bot commented May 6, 2023

github-actions bot commented May 6, 2023

hugo-sid left a comment • edited Loading

Choose a reason for hiding this comment

hugo-sid May 6, 2023 • edited Loading

Choose a reason for hiding this comment

hugo-sid May 6, 2023

Choose a reason for hiding this comment

kusyka911 May 16, 2023 • edited Loading

Choose a reason for hiding this comment

hugo-sid May 16, 2023 • edited Loading

Choose a reason for hiding this comment

hugo-sid May 21, 2023

Choose a reason for hiding this comment

github-actions bot commented May 21, 2023

github-actions bot commented May 21, 2023

sonarcloud bot commented May 21, 2023

hugo-sid left a comment

Choose a reason for hiding this comment

netlify bot commented May 5, 2023 •

edited

Loading

kusyka911 May 5, 2023 •

edited

Loading

hugo-sid May 6, 2023 •

edited

Loading

kusyka911 May 6, 2023 •

edited

Loading

hugo-sid May 6, 2023 •

edited

Loading

hugo-sid left a comment •

edited

Loading

hugo-sid May 6, 2023 •

edited

Loading

kusyka911 May 16, 2023 •

edited

Loading

hugo-sid May 16, 2023 •

edited

Loading