-
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?
Changes from 17 commits
a8e3ae3
6c8ce62
35d9723
713cb6b
df4ace5
ca546dc
d34a028
061354a
2249592
12d3176
47c911b
45aab55
c86d8ee
ff8cac7
8480f42
b403b16
1578e54
bea8b65
f924237
185a893
3086c3b
8e24cad
48cc2a9
3c0435d
a417f80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,10 @@ | ||
import { Box, Container, Typography } from "@mui/material"; | ||
import { WizardForm } from "@newm-web/elements"; | ||
import { FormikHelpers, FormikValues } from "formik"; | ||
import { FunctionComponent, useEffect } from "react"; | ||
import { useNavigate } from "react-router-dom"; | ||
import { FunctionComponent, useEffect, useState } from "react"; | ||
import { useLocation, useNavigate } from "react-router-dom"; | ||
import * as Yup from "yup"; | ||
import { PageNotFound } from "@newm-web/components"; | ||
import AdvancedSongDetails from "./AdvancedSongDetails"; | ||
import BasicSongDetails from "./BasicSongDetails"; | ||
import ConfirmAgreement from "./ConfirmAgreement"; | ||
|
@@ -28,6 +29,9 @@ export interface UploadSongFormValues extends UploadSongThunkRequest { | |
|
||
const UploadSong: FunctionComponent = () => { | ||
const navigate = useNavigate(); | ||
const location = useLocation(); | ||
|
||
const [isValidPath, setIsValidPath] = useState(true); | ||
|
||
const { data: genres = [] } = useGetGenresQuery(); | ||
const { data: roles = [] } = useGetRolesQuery(); | ||
|
@@ -139,17 +143,37 @@ const UploadSong: FunctionComponent = () => { | |
|
||
/** | ||
* Ensure user is returned to first step on refresh since form | ||
* contents are not persisted. | ||
* contents are not persisted. If user navigates to an invalid | ||
* path, redirect to 404 page. | ||
* | ||
* TODO: remove this when form values are persisted on refresh | ||
* TODO: remove the navigation when form values are persisted on refresh | ||
*/ | ||
useEffect(() => { | ||
navigate("/home/upload-song", { replace: true }); | ||
const validPaths = [ | ||
"/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 commentThe 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.
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 commentThe 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 commentThe 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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I refactored the wizard routes and check for valid routes to mimic your example, it should be a little bit cleaner and should reduce future conflicts on updating the routes. |
||
|
||
// Remove trailing slashes to compare paths | ||
const normalizePath = (path: string) => path.replace(/\/+$/, ""); | ||
const currentPath = normalizePath(location.pathname); | ||
|
||
if (!validPaths.includes(currentPath)) { | ||
setIsValidPath(false); | ||
} else { | ||
setIsValidPath(true); | ||
navigate("/home/upload-song", { replace: true }); | ||
} | ||
// useNavigate doesn't return memoized function, including it | ||
// as dependency will run this when navigation occurs. Exclude | ||
// to only run on mount. | ||
// eslint-disable-next-line | ||
}, []); | ||
}, [location.pathname]); | ||
|
||
if (!isValidPath) { | ||
return <PageNotFound redirectUrl="/home/upload-song" />; | ||
} | ||
|
||
const validations = { | ||
agreesToCoverArtGuidelines: commonYupValidation.agreesToCoverArtGuidelines, | ||
|
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
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.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 removed the useEffect and went for the useMemo route, it cleaned up code and simplified the logic a bit as well. Copilot said there shouldn't be any large rerender performance issues from doing it this way. Let me know if you see any issues with the update on your end.