updates #487 - update pages routes #485
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/c0d3/c0d3-app/q5tumk81w |
Codecov Report
@@ Coverage Diff @@
## master #485 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 97 97
Lines 1342 1355 +13
Branches 290 294 +4
=========================================
+ Hits 1342 1355 +13
|
| @@ -12,7 +12,7 @@ export type User = { | |||
| } | |||
|
|
|||
| export type Lesson = { | |||
| id: number | |||
| id: string | |||
There was a problem hiding this comment.
See #486. Such small change doesn't warrant a separate PR.
pages/404.tsx
Outdated
| export default function NotFound() { | ||
| return <Error statusCode={404} /> | ||
| return <Error title="404 error" src="/404.png" message="Page not found" /> |
There was a problem hiding this comment.
Next.JS has a default 404 page. Please utilize this instead of creating a 404 page.
There was a problem hiding this comment.
I don't understand, according to docs next.js suggests editing pages/404.tsx to create a custom error page. Do you want me to modify next.js error component instead of creating my own?
There was a problem hiding this comment.
You're right. I thought next.js uses a different syntax than 404 for page. That part is fine.
However, it's better to override next.js error component rather than creating a 500 page. Otherwise, users won't be redirected to 404 when they go to c0d3.com/500 for whatever reason.
There was a problem hiding this comment.
Otherwise, users won't be redirected to 404 when they go to c0d3.com/500 for whatever reason.
c0d3.com/500 won't redirect to 404 page (check vercel), but you're right, I forgot to update _error page. I edited it and it should display new Error screens too.
pages/500.tsx
Outdated
| import React from 'react' | ||
| import Error from '../components/Error' | ||
|
|
||
| export default function NotFound() { |
There was a problem hiding this comment.
A status code of 500 is a server error and isn't not found. This function name is misleading.
There was a problem hiding this comment.
Looks reasonable, will be fixed.
pages/curriculum/[lesson].tsx
Outdated
| } | ||
| if (!lessons || !alerts) { | ||
| return ( | ||
| <Error title="Internal server error" message="Bad data" src="/500.png" /> |
There was a problem hiding this comment.
Shouldn't this mean the user is not logged in?
There was a problem hiding this comment.
No session means that user is not logged in. This case happens if there is a session but no lessons or no alerts.
There was a problem hiding this comment.
Wouldn't it better to redirect to login if this is the case?
There was a problem hiding this comment.
I think it would be more confusing. Since there is nothing wrong with session already authorized user could be redirected to login page. He relogins, login redirects him to curriculum page, no lessons/alerts, he is on log in page again.
This way he'll get a 500 error screen and won't waste time logging in/logging out.
pages/curriculum/[lesson].tsx
Outdated
| ) | ||
| if (!currentLesson) { | ||
| return <Error title="404 error" message="Page not found" src="/404.png" /> |
There was a problem hiding this comment.
404 should be reserved for the page is not found. Not an error.
There was a problem hiding this comment.
This case happens if user tries to open lessonID which does not exist, something like https://www.c0d3.com/curriculum/100ShouldNotExist. I could redirect him to 404 page but this way he won't be able to check his incorrect url since it's instantly replaced by c0d3/404.
Do you want me to rename Error component for something else?
pages/_error.tsx
Outdated
| const statusCode = res ? res.statusCode : err ? err.statusCode : 404 | ||
| const errorInitialProps = { | ||
| statusCode, | ||
| hasGetInitialPropsRun: false |
There was a problem hiding this comment.
On line 40, you set this value to true. So it's redundant here
…o Error/routes
#484 needs to be merged first. Part of #467.
This PR updates page routes. If user is unauthorized he will be redirected to login page. If the page does not exist he will get 404 Error. Graphql error will result in 500 Internal Server Error.