Route based navigation

[anttimaki]

No description provided.

[ebkr]

Only one comment requiring explanation. Otherwise looks good and functions fine

[MythicManiac]

I stopped reviewing around 50% through since I realized the route structure is a bit weird as it is.

Did you consider changing the route structure in a way where all of the views with a navbar would be child routes of a navbar view? There's no reason we'd have to unmount & re-mount the navbar between route changes after all, when we're really just changing the sub-view.

In the current route setup, some views already use that kind of a setup, but some don't. It would probably be smart to have all of the views that use a navbar be children of the view that includes the navbar. That way there doesn't need to be any implicit layout inherited from the root view, and we can simply mount the router view directly and trust the structure is provided by nested routers.

Not sure if this explanation is clear enough, but TL;DR: is that I don't think it's a good idea to assume (in App.tsx) how the routes are structured, and it'd be better to use the pattern of one component per route & let the routes sort it out themselves by leveraging sub-routes.

[anttimaki]

I think I understand what you mean but I can't be 100% certain.

Did you consider changing the route structure in a way where all of the views with a navbar would be child routes of a navbar view?

I didn't, simply because the thought didn't occur to me. Most likely because in traditional website sense it would be very weird to structure the URLs/routes that way, but I guess it would be ok here since the users aren't exposed to the URLs anyway.

There's no reason we'd have to unmount & re-mount the navbar between route changes after all, when we're really just changing the sub-view.

True, but if it's the performance implications of un/remount that worries you, I wouldn't see it as an issue that should be prioritised over the code's "ease of use".

It would probably be smart to have all of the views that use a navbar be children of the view that includes the navbar

Includes the navbar, or includes a <router-view> that can be used to define the navbar component? The former assumes there's just on navbar per app, which may or may not be true, IDK. The latter offers more flexibility but might corrode the trust in provided structure you mention.

I don't think it's a good idea to assume (in App.tsx) how the routes are structured

I don't know how much it helps to move the assumptions one level down to "semi-root" components. I guess it's nice idea, even if it doesn't change things much in practice.

(I guess the point I made for the quote above this one could be evaded by having more than two semi-root level components, where one doesn't have a navbar and others have different navbars.)

use the pattern of one component per route & let the routes sort it out themselves by leveraging sub-routes.

This does sound good, but my guess is that it would require non-trivial amount of work, e.g. there's still views where subviews are implemented as "tabs" of the parent view, like the local/online mods subviews were in the manager view before the recent changes.

---

In conclusion, I'm up to exploring the ideas above in more detail, but the question is: should it be done as a part of this PR and/or milestone? Possible problems are that this PR soft blocks the changes already in develop from moving to TSMM, and messing with App.vue more than we already do might cause conflicts with the settings-loader PRs. Also this PR, as it is, fulfills the related milestone's DoD, so adding bigger changes feels out of scope.

Let me know what you think @MythicManiac

[MythicManiac]

Most likely because in traditional website sense it would be very weird to structure the URLs/routes that way, but I guess it would be ok here since the users aren't exposed to the URLs anyway.

Yeah that's true, it's a fairly common pattern on SPAs however.

should it be done as a part of this PR and/or milestone?

The reason I brought this up is because I saw a lot of views without navbar require changes due to the implicit higher structure inherited from App.tsx, which seemed a bit weird to me. Rephrased a bit, I think there are a lot of changes in this PR that should not be necessary, primarily on the navbar-less views. In the context of minimizing the changes of this PR rather than completely overhauling the routes, I don't think you'd have to go all the way (e.g. the tabs that were mentioned not following routes yet in some cases), but I'd imagine moving all the navbar-enabled views under the same parent & leaving the other views alone would make for a simpler change overall (correct me if I'm wrong).

Also this PR, as it is, fulfills the related milestone's DoD, so adding bigger changes feels out of scope.

So the above in mind, I'd think it's less changes & a simpler change overall if a single root level router is retained, and all of the navbar-enabled views are sorted under the same parent/child hierarchy as some of them already are. It might require extracting a "cleaner" navbar-view parent if the current one has too much going on in it, I'm not entirely sure.

[MythicManiac]

Looks good to me, feel free to merge based on what was discussed in Discord