Fix redirection issues in coach resource selection side panels #13223
Conversation
github-actions
bot
added
APP: Coach
| import { onBeforeRouteUpdate, useRouter } from 'vue-router/composables'; | ||
|
|
||
| /** | ||
| * Composable for providing a route tracking context to track the previous route. You can |
There was a problem hiding this comment.
Does this composable makes sense? The benefit of having this like this is that if I tracked the routes higher in the routing hierarchy as for example in the coach app.js, then I would need to validate if the previous route belongs to the routes related to the side panel and that seems like adding a little bit more complexity.
There was a problem hiding this comment.
It makes sense to me be used like this where it's scoped to a particular sub-hierarchy in the code. It does seem like it's something that is specific to the side panel use case - do you anticipate this being used in other contexts as well?
There was a problem hiding this comment.
For now, yes, just for side panels, but it would work on any sub-route context with scoped navigation
Build Artifacts
|
|
|
||
| if (getFallbackRoute) { | ||
| router.push(getFallbackRoute()); | ||
| } |
There was a problem hiding this comment.
Might be worth console.error-ing or throwing an error here so we don't run into it silently failing to go back without warning.
There was a problem hiding this comment.
Sounds good! Thanks @nucleogenesis!
| import { onBeforeRouteUpdate, useRouter } from 'vue-router/composables'; | ||
|
|
||
| /** | ||
| * Composable for providing a route tracking context to track the previous route. You can |
There was a problem hiding this comment.
It makes sense to me be used like this where it's scoped to a particular sub-hierarchy in the code. It does seem like it's something that is specific to the side panel use case - do you anticipate this being used in other contexts as well?
| function goBack() { | ||
| // Go back just if there is a previous route that belongs to the | ||
| // same routes context. | ||
| if (previousRoute.value) { |
There was a problem hiding this comment.
This will break if someone refreshes the page, as previousRoute.value will be reinitialized to null - I think this would work just as well if we checked window.history.length > 1 instead?
That would also mean we don't need to do any provide inject here, and this function could be exposed directly from a reusable composable.
There was a problem hiding this comment.
This will break if someone refreshes the page, as previousRoute.value will be reinitialized to null
It would use the fallback route when that happens!
I think this would work just as well if we checked window.history.length > 1 instead?
Its not totally equivalent. This method provides two "guards" here, one is if a user copy an url and paste it in another tab that has already some history there. Then if they click on go back, it will redirect the page to the external page they were before. This is a very edge case that if we dont want to support its okay 😅.
The other case is what would we expect to happen when we click, for example, on "replace question" and we get redirected to the exercise preview and then we click on go back? would we expect the side panel to close, or to go to the parent folder? The first time I tried I unconsciously expected it would take me to the parent folder, but any other perspective is welcome :). For that we would need to check the previous route to check where is it coming from. And the intention of making it a composable was to reutilize this logic, but unfortunately vue-router doesnt expose anything like a onBeforeRouteEnter for composables :/ because of that limitation is why I needed to track these routes in an onBeforeRouteUpdate and have set the previous route in the root of the sub-routes context. To reuse this logic without the provide inject for it to be reusable we would need to use mixins as there we would be able to introduce the beforeRouteEnter.
I tried to build something reusable without having to write the beforeRouteEnter each time that we wanted to access the prev route and avoiding the mixins. But if this adds too much complexity, we can just get back to the beforeRouteEnter pattern and just use the router.back method instead of the router.push and that would solve the most of the problems reported in #13073 aswell :).
There was a problem hiding this comment.
Yes, sorry, the fallback route does provide some coverage, even if at the loss of some state on refresh.
I think this is fine to merge as is - as we implement side panel workflows in more places in later releases we can revisit whether this is something we want to reuse in this way, or have a simpler implementation.
The more general thing I am thinking about is the contrast with Learn where we encode the full back route in the URL itself (at least partly because we anticipate links from Learn being shared more frequently) - but it does cause a horribly bloated URL, and can encode two or three steps of navigation in it too. I don't think I'd want to lose some of that state, just because of the immersive modal nature of the navigation there, rather than a side panel, but worth reflecting on what our more general story for "back arrow" navigation is, both from a user experience, and a technical perspective!
| } | ||
|
|
||
| // eslint-disable-next-line no-console | ||
| console.warn('No fallback route provided to navigate back. No action taken.'); |
There was a problem hiding this comment.
Oh, sorry, one thing I should have caught - you should use the kolibri-logging library here instead of disabling linting...
Summary
The previous pattern to redirect back used to store the previous route, and when the user clicked on a back button this pushed the previous route into the history stack. Now we are leveraging this to the router.back() method that pops out the current route from the history stack to go back, but we only do this if the previous route was in the same sub-routes context, this prevents the user to exit the current sub-routes context (e.g. exit the side panel) if they were previously redirected from anywhere outside the side panel (This even prevents that if the user had an external page opened before, we dont redirect back to that external page).
Compartir.pantalla.-.2025-03-18.10_01_57.mp4
References
Closes #13073
Reviewer guidance
Note:
I have wear my user hat and have tried to follow what my user instict was expecting when I clicked on a goBack button, but does it makes sense? Feel free to question if a back button dont redirect you to where you were expecting.