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

page-router: Only replacing main can result in wrong content #141

Closed
nobkd opened this issue Dec 31, 2023 · 2 comments
Closed

page-router: Only replacing main can result in wrong content #141

nobkd opened this issue Dec 31, 2023 · 2 comments
Assignees
@tipiirai
Labels
bug

Comments

@nobkd
Copy link
Collaborator

nobkd commented Dec 31, 2023 •
edited

nue/packages/nuekit/src/browser/page-router.js

Lines 14 to 23 in 854f20f

// main / body
const main = $('main')
const main2 = $('main', dom)
if (main && main2) {
main.replaceWith(main2)
} else {
$('body').innerHTML = $('body2', dom).innerHTML
$('body').classList = $('body2', dom).classList
}

The code above is insufficient, if for example I have changing content in my header (e.g. I add a current class to the current pages link)

I patched it locally by just using the content of the else statement, but this is most likely not the best strategy.

My code diff 
-  // main / body
-  const main = $('main')
-  const main2 = $('main', dom)
-
-  if (main && main2) {
-    main.replaceWith(main2)
-  } else {
-    $('body').innerHTML = $('body2', dom).innerHTML
-    $('body').classList = $('body2', dom).classList
-  }
+  $('body').innerHTML = $('body2', dom).innerHTML
+  $('body').classList = $('body2', dom).classList

Idk... Maybe analyzing if the content of header and/or footer is static on build time is the way to go?

Just realized, that the same also goes for the <head>. E.g. description or OG image, etc.
@tipiirai
Copy link
Contributor

tipiirai commented Jan 3, 2024

True. This is indeed the case with the current implementation. My goal is to only replace main if the surrounding elements are not changed: this avoids unneeded rendering, allows better animation support and toggling the "selected" class name for links. Obviously the goal should also be to fix the issues you brought up. One potentially hacky way could be to compare outerhHTML properties of the old/new header,footer,head elements update accordingly. Thoughts?

@tipiirai tipiirai self-assigned this Jan 9, 2024
@tipiirai tipiirai added the bug label Jan 9, 2024
@tipiirai
Copy link
Contributor

tipiirai commented Jan 15, 2024 •
edited

Found a nice solution to this one. It now studies the old/new page and only updates if there are changes. Checks all landmark elements: body, header, main, and footer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tipiirai tipiirai

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@tipiirai @nobkd