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.

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: Alters main menu selection to not cause full re-render #2766

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Jul 17, 2024

Back in #2662 we moved the main navigation in App.vue to use route.child() for selection.

I noticed that this is causing a full page re-render whenever the route is changed. You can notice this because our Github button in the header flickers on every single navigation (caused by the re-render). When navigating between tabbed sub-pages you shouldn't see a flicker (but you will do when navigation between Home, Zones, Meshes).

Strangely route.child() doesn't cause full page re-renders in other areas, in fact it was used/made specifically to avoid full page re-renders.

For the moment I've added a fix straight into App.vue that avoids the issue. But I would guess at some point I will come back to try and ensure that route.child() or some iteration of it works consistently across all usages.

Copy link

netlify bot commented Jul 17, 2024

Deploy Preview for kuma-gui ready!

Name Link
🔨 Latest commit aec5c04
🔍 Latest deploy log https://app.netlify.com/sites/kuma-gui/deploys/669780ee6d3ab700088f4a30
😎 Deploy Preview https://deploy-preview-2766--kuma-gui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@johncowen johncowen marked this pull request as ready for review July 17, 2024 14:55
@johncowen johncowen requested a review from a team as a code owner July 17, 2024 14:55
@johncowen johncowen added this to the 2.9.x milestone Jul 18, 2024
@johncowen johncowen self-assigned this Jul 18, 2024
@johncowen
Copy link
Contributor Author

Wanted to note that this PR is good and its a good fix for the regression explained in the PR desc, but!

You can notice this because our Github button in the header flickers on every single navigation (caused by the re-render).

I noticed that only the Github button has its DOM elements re-rendered. Whilst this was a good indication that we were re-calculating the rendering of things that we shouldn't. The DOM for the GH button shouldn't get re-rendered at all following this recalculation. I took a look at the vue-github-buttons package that we use for this and I think its the way that the vie implementation is handled. The vue implementation further uses another github-buttons package, so theoretically we could replace the Vue implementation with our own that doesn't have a needless re-render, but still depend on github-buttons, which would be a small but worthwhile technical improvement.

I'll probably PR something to cover ^ at some point soon.

@johncowen johncowen merged commit 39de583 into kumahq:master Jul 25, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants