Improve 404 handling #310
Improve 404 handling #310
Conversation
parostatkiem-zz
requested review from
antiheld,
bszwarc,
dariadomagala-sap,
hardl,
jesusreal,
kazydek,
klaudiagrz,
kwiatekus,
maxmarkus,
pekura,
tomekpapiernik and
y-kkamil
as code owners
There was a problem hiding this comment.
One additional bug that I found during the review - when going to 'partially wrong link' from the browsers address bar (not using Luigi client), the frame is loaded twice. I think it works the same way on master, so it's not your implementations fault, although maybe it would be good to fix it within this task.
core/src/services/routing.js
Outdated
| @@ -264,3 +270,24 @@ export const handleRouteClick = node => { | |||
| navigateTo(route); | |||
| } | |||
| }; | |||
|
|
|||
| const ShowNotExactRouteError = async ( | |||
There was a problem hiding this comment.
All variables start with lowercase letter, so could you please rename it to showNotExactRouteError to consistency?
core/src/services/routing.js
Outdated
| } | ||
| //the path is unrecognized at all and cannot be fitted to any known one | ||
| const custom404handler = LuigiConfig.getConfigValue( | ||
| 'settings.handle404' |
There was a problem hiding this comment.
I would rename handle404 since all functions in our config are nouns. 404handler or something like that.
There was a problem hiding this comment.
And I think that 404 handler should be part of navigation, not settings?
There was a problem hiding this comment.
404handler was the first name I thought about but variable names can't start with number.
I decided to place it in settings because, in my opinion, it's not related to navigation. Any other suggestions about the name? I've got no idea what I should change it to :D
core/src/services/routing.js
Outdated
|
|
||
| component.set({ alert }); | ||
| navigateTo('/'); | ||
| if (defaultChildNode && pathData.navigationPath.length > 2) { |
There was a problem hiding this comment.
What is this pathData.navigationPath.length > 2 for?
There was a problem hiding this comment.
- In case (probably impossible)
pathData.navitationPathwas empty, there will be no errors. - I've just changed it to
1becauseoverviewis a defaultChild for/. If it was set to >0 or not present at all, "Could not find exact route" error would appear instead of "Could not find requested route" due to automatic redirection to/.
Thanks for noticing 2 was the wrong number 👍
There was a problem hiding this comment.
I would rather create a variable out of pathData.navigationPath.length > 1 to make clear what we are checking here through the variable name.
|
core/src/App.html
Outdated
|
|
||
| window.addEventListener('popstate', async e => { | ||
| const alert = this.get().alert; | ||
| if (alert && alert.ttl !== undefined) { |
There was a problem hiding this comment.
I would use typeof number here, just to be save.
There was a problem hiding this comment.
I'd love to use isInteger() but Internet Explorer's compatibility blocks me from doing this :(
| @@ -320,6 +320,11 @@ var projectsNavProviderFn = function(context) { | |||
| }); | |||
| }; | |||
|
|
|||
| var customPageNotFoundHandler = function(wrongPath, wasAnyPathFitted) { | |||
There was a problem hiding this comment.
Maybe it would be good to add to documentation that this function might receive wrongPath and wasAnyPathFitted?
core/src/services/routing.js
Outdated
| : 'Could not find the requested route', | ||
| link: notFoundPath, | ||
| ttl: pathToRedirect === '/' ? 2 : 1 //how many redirections the alert will 'survive'. | ||
| //Redirecting to '/' causes another redirection to '/overview', that's why TTL needs to be bigger |
There was a problem hiding this comment.
In our case it's ok, because we don't have a view for '/', so it redirects to '/overview', but what if someone has a view on '/' and doesn't have this additional redirection?
Co-Authored-By: parostatkiem <[email protected]>
Co-Authored-By: parostatkiem <[email protected]>
Improve 404 handling
Description
lerna bootstraprequired!Changes proposed in this pull request:
ttlproperty which lets it survive n redirects (set to 1)Could not map the exact target...error appears when user tries to enter a route with a defaultChildNode at the end and he's redirected to that node.extendedConfiguration.js::583to be able to see it)Related issue(s)
Resolves #310, #281