Closed EricosEagle closed 4 months ago
salemlf
commented
4 months ago I've been noticing this issue with the popup too! Not sure exactly how it was introduced. Navigation blocking is a pain, so the current solution is somewhat hacky. I'm in the process of migrating to a different router to solve this issue (and other problems), but this is great to fix in the meantime! I'll take a look over the changes you made 😊
EricosEagle
commented
4 months ago Hey, is there anything else I should change? If not, I think this should be merged ASAP as the bug is very noticeable and disruptive.
EricosEagle
commented
4 months ago Hey, is there anything else I should change? If not, I think this should be merged ASAP as the bug is very noticeable and disruptive.
An idea that came to mind is that rather than saying which paths we don't want to block, we can explicitly state which ones we do (from what I can tell, we only want to block moving back to the homepage from a lesson quiz or review session), in order to increase performance. But as you are planning to migrate to a new router soon I'm not sure how much time I should invest.
EricosEagle
commented
4 months ago Hey, is there anything else I should change? If not, I think this should be merged ASAP as the bug is very noticeable and disruptive.
An idea that came to mind is that rather than saying which paths we don't want to block, we can explicitly state which ones we do (from what I can tell, we only want to block moving back to the homepage from a lesson quiz or review session), in order to increase performance. But as you are planning to migrate to a new router soon I'm not sure how much time I should invest.
Added this as a new commit, if you prefer this method I will remove the first commit, otherwise I will restore the code to what it was before.
salemlf
commented
4 months ago Hey, is there anything else I should change? If not, I think this should be merged ASAP as the bug is very noticeable and disruptive.
An idea that came to mind is that rather than saying which paths we don't want to block, we can explicitly state which ones we do (from what I can tell, we only want to block moving back to the homepage from a lesson quiz or review session), in order to increase performance. But as you are planning to migrate to a new router soon I'm not sure how much time I should invest.
That's a good idea, only allowing specific paths totally makes sense! This is basically how I have it implemented in the version with the new router. I'm just finishing up migrating all tests to use the new router setup, so should have that all done within the week. I'll take a look at this PR after work, would be good to have a fix for this issue in the meantime 😊
I noticed that I started to get a popup that blocks me from moving from the summary to the homepage, so I exempted it from the blocking function. I also replaced the regex with
startsWith/endsWithto make the code a bit clearer and faster. Finally, I reduced the animation time of the wrapping up popup as I felt that it was a bit too long.