~kvo/taskcollect-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
2

[PATCH] Updated script to only show loading animation when clicking navigation items and general resource pages.

Details
Message ID
<20240406234515.5267-1-al0f@envs.net>
DKIM signature
pass
Download raw message
Patch: +44 -11
From: al2f <al2f@noreply.codeberg.org>

---
 res/script.js | 55 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 11 deletions(-)

diff --git a/res/script.js b/res/script.js
index eb463af..c1be9f2 100644
--- a/res/script.js
+++ b/res/script.js
@@ -2,23 +2,56 @@ function toggleMobileMenu(menu) {
  menu.classList.toggle("open");
}

// Replace the whole page body but the tabs with the loading animation.
document.addEventListener('click', function (event) {
  var loaderBody = `<div class="loader-container">
function applyLoader(){
	var loaderBody = `<div class="loader-container">
<div class="loader" scale=1></div>
<div>`
  if (["A", "BUTTON"].includes(event.target.tagName)) {
	// Skip middle clicks and ctrl+clicks	
	if (event.ctrlKey || event.which == 2) return;
	// Skip linked resource inside list
	if (event.target.parentNode.tagName == "LI") return;
	
    // Select everything that isn't the navigation bar and remove it.
    var divs = document.querySelectorAll("body > *:not(header)");

	// Select everything that isn't the navigation bar and remove it.
    var divs = document.querySelectorAll("body > *:not(nav)");
    for (i = 0; i < divs.length; i++) {
      divs[i].remove();
    }
    // Add the loader div the body of the HTML that's left.
    document.body.innerHTML += loaderBody;
}

// Replace the whole page body but the tabs with the loading animation.
document.addEventListener('click', function (event) {
  
  if (["A", "BUTTON"].includes(event.target.tagName)) {
	
	// Skip middle clicks and ctrl+clicks	
	if (event.ctrlKey || event.which == 2) return;
	
	// Show loading animation when clicking on links in navigation	
	var navs = document.getElementsByTagName("nav")
	console.log(navs)
	if (navs != null){
		var nav = navs[0]
		console.log(nav)
		if ( nav != null ){
			if (nav.contains(event.target)){
				applyLoader()
			}
		}
	}
	
	// Show loading animation when cliking on resource/task links
	var first_parent = event.target.parentElement
	console.log(first_parent.tagName)
	if(first_parent != null){
		if(first_parent.tagName == "P"){
			var second_parent = first_parent.parentElement
			if(second_parent != null){
				if(second_parent.tagName == "DIV"){
					var third_parent = second_parent.parentElement
					if(third_parent.tagName == "DETAILS"){
						applyLoader()
					}
				}
			}
		}
	}
  }
}, false);
-- 
2.44.0
Details
Message ID
<D0EOY13RYYPP.2NEL2MIJM4KWM@joybook>
In-Reply-To
<20240406234515.5267-1-al0f@envs.net> (view parent)
DKIM signature
pass
Download raw message
Content-wise the patch looks good, except the `console.log` statements
that seems to have remained after debugging. Those statements should
be removed.

However, some of the if statements could be made more concise. Seeing
as JavaScript logical expressions evaluate left-to-right, you can
write `if (nav != null && nav.contains(event.target))` because the
second statement after the `&&` will not be evaluated if the first
statement is already false.

Aside from that my only concerns are about styling. This is not a big
issue, and the styling was inconsistent in the JS script before as
well, but it would be nice to resolve the inconsistency now that we're
already in the process of modifying the script.

Indentation is the most inconsistent part of the JS file. There are
intermingled tabs and spaces throughout. I think it would be better to
stick with the official MDN web docs guidelines and use 2 spaces for
indentation in JS, even though I think 2 space indentation is
absolutely bizarre. Also, I think `loaderBody` could probably be moved
outside the function as a global variable so that it doesn't make the
indentation look jumbled. The other option would be to use `let`
instead of `var` and declare the `loaderBody` string in one line
within the function.

Semicolon usage is inconsistent. I think it's probably better to use
semicolons throughout the entire file. Another inconsistent thing
about syntax is spacing in control blocks. Prefer this format:
`if (navs != null) {` over other approaches to spacing between
keywords and braces.

Assuming JS doens't have anything against this, would it be possible
to change "A", "BUTTON", etc. to lowercase?

Although this is not an inconsistency in the patch, JavaScript tends
to use camel case, so something like `firstParent` would be better
than `first_parent`. My personal opinion is that `parent`,
`grandparent`, and `greatGrandparent` are more intuitive, even though
the last variable is a bit too long for my taste.

Sorry that this review email is so long. The patch is good though;
once the logging/styling issues are sorted I'll merge it into main.
Details
Message ID
<D0M9PLJA4FNC.279V7KZ65GWLP@redback>
In-Reply-To
<D0EOY13RYYPP.2NEL2MIJM4KWM@joybook> (view parent)
DKIM signature
pass
Download raw message
I've merged the patch into main and applied the fixes. Issue #15 can
now be marked as resolved.
Reply to thread Export thread (mbox)