-
Notifications
You must be signed in to change notification settings - Fork 6
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
STUD-300: Implement 404 "Page Not Found" error page for NEWM Studio #848
base: master
Are you sure you want to change the base?
STUD-300: Implement 404 "Page Not Found" error page for NEWM Studio #848
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good but had some comments.
"/home/upload-song", | ||
"/home/upload-song/advanced-details", | ||
"/home/upload-song/confirm", | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach could create some issues, as we'll need to make sure to always update this array when adding a new route, or it could result in the 404 component being incorrectly displayed. Instead, it would be better to declare the routes variable externally and then derive this array from them. E.g.
const routes = [{
path: "advanced-details",
...
}]
const validRoutes = routes.map((route) => `/home/upload-song/${route.path}`)
...
if (!isValidPath) {
return <PageNotFound redirectUrl="/home/upload-song" />;
}
return (
<WizardForm
routes={routes}
...
That way it only has to be updated in one place, when the routes are declared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 404 redirect functionality could also be included in the WizardForm component, although I feel less strongly about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to consolidate the routes, will make those changes.
I was trying to keep to the highest level where the routing logic was made. WizardForm is used in multiple different contexts so it felt off to integrate any triggering there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't have a strong opinion on having the logic in the WizardForm. There are pros and cons to doing it that way.
|
||
if (!isValidPath) { | ||
return <PageNotFound />; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this useEffect? Couldn't we just use
if (!isValidPath) {
return <PageNotFound />;
}
directly? It would make the code a bit cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to remove the useEffect and convert the valid path conditional into a function? I will play around with removing the useEffect and see how the rendering is affected by it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I supposed a benefit of having it in a useEffect
call is that it would only run once. It could be fine to keep it there, on second thought.
Add:
Update:
Note: