Added custom 404 page #410

Merged
snedadah merged 12 commits from 404page into main 2022-03-28 14:53:43 -04:00
Owner
Closes #282 demo: https://csclub.uwaterloo.ca/~a3thakra/csc/404page/404/
snedadah added 5 commits 2022-03-10 23:57:16 -05:00
continuous-integration/drone/push Build is passing Details
8486160c2a
added 404 page and styling
continuous-integration/drone/push Build is failing Details
b723a0cde0
fixed formatting of 404 text
continuous-integration/drone/push Build is passing Details
3b02b0b5db
escaped apostroph's
continuous-integration/drone/push Build is passing Details
7192fe6681
updated codey
continuous-integration/drone/push Build is passing Details
6af42d77b7
Empty Commit
snedadah requested review from n3parikh 2022-03-11 00:57:08 -05:00
snedadah requested review from j285he 2022-03-11 00:57:08 -05:00
snedadah requested review from a258wang 2022-03-11 00:57:09 -05:00
Author
Owner

there is a slight problem with the build on the staging enviroment, that the 404 page isnt actually used when vising an unknown page.

I think this is beacuse when you visit an undefined page, it is using the 404 page of the normal site, instead of this branch.

However, on local builds it works fine, thus I believe it would work as intended on the normal site?

there is a slight problem with the build on the staging enviroment, that the 404 page isnt actually used when vising an unknown page. I think this is beacuse when you visit an undefined page, it is using the 404 page of the normal site, instead of this branch. However, on local builds it works fine, thus I believe it would work as intended on the normal site?
n3parikh reviewed 2022-03-15 04:54:31 -04:00
n3parikh left a comment
Owner

A few minor comments

A few minor comments
@ -0,0 +1,15 @@
.container{
Owner

Add space between r and { (and also for the codey class)

Add space between `r` and `{` (and also for the `codey` class)
snedadah marked this conversation as resolved
@ -0,0 +1,15 @@
.container{
display:flex;
Owner

Use 2 spaces for indentation rather than tab.

Use 2 spaces for indentation rather than tab.
pages/404.tsx Outdated
@ -0,0 +9,4 @@
return (
<div className={styles.container}>
<div className={styles.text}>
<Title>Error 404</Title>
Owner

I think title should be a bit more descriptive. Maybe something like "404: Page Not Found"?

I think title should be a bit more descriptive. Maybe something like "404: Page Not Found"?
snedadah marked this conversation as resolved
pages/404.tsx Outdated
@ -0,0 +11,4 @@
<div className={styles.text}>
<Title>Error 404</Title>
<h2>Error 404</h2>
<h1>We couldn&apos;t find the page you&apos;re looking for!</h1>
Owner

We use on the get involved page, let's do the same thing here for consistency (and we also won't need to escape it in that case).

We use `’` on the [get involved page](https://git.csclub.uwaterloo.ca/www/www-new/src/commit/6af42d77b7b73ef950ce68930000e3263b3bb17e/pages/get-involved.tsx), let's do the same thing here for consistency (and we also won't need to escape it in that case).
snedadah marked this conversation as resolved
j285he approved these changes 2022-03-16 18:06:08 -04:00
pages/404.tsx Outdated
@ -0,0 +14,4 @@
<h1>We couldn&apos;t find the page you&apos;re looking for!</h1>
<p>
We&apos;re working on it, but in the meantime,
<Link href="/"> click here to go back to the main page.</Link>
Member

Very minor thing, but when you tab over this link it includes the space before "click". I think it would look better if we didn't include the space in the link

Very minor thing, but when you tab over this link it includes the space before "click". I think it would look better if we didn't include the space in the link
snedadah marked this conversation as resolved
snedadah added 1 commit 2022-03-23 12:50:30 -04:00
continuous-integration/drone/push Build is passing Details
fe4b699686
made slight adjustments to 404 page
snedadah added 1 commit 2022-03-23 13:12:51 -04:00
continuous-integration/drone/push Build is passing Details
a77aada95b
reverted heading change
Owner

Some comments:

  1. The Codey svg seems to have some extra whitespace in it, particularly on the left side, so it doesn't look centred on the mobile version of the page. Would it be possible to change the svg to get rid of that empty space?
  2. This isn't a merge-blocking thing, but we may want to use a custom shapes background (or get rid of the shapes) on the mobile 404 page, since they are interfering slightly with the text.

image

Some comments: 1. The Codey svg seems to have some extra whitespace in it, particularly on the left side, so it doesn't look centred on the mobile version of the page. Would it be possible to change the svg to get rid of that empty space? 2. This isn't a merge-blocking thing, but we may want to use a custom shapes background (or get rid of the shapes) on the mobile 404 page, since they are interfering slightly with the text. ![image](/attachments/200b753a-3506-4f92-bbb0-3a6e4a81cace)
snedadah added 1 commit 2022-03-23 16:52:59 -04:00
continuous-integration/drone/push Build is passing Details
efcdd7ef52
trimmed 404 codey graphic
snedadah added 1 commit 2022-03-26 01:21:20 -04:00
continuous-integration/drone/push Build is failing Details
54c4dcd2ed
adjusted spacing of 404 page
snedadah added 1 commit 2022-03-26 01:22:16 -04:00
continuous-integration/drone/push Build is passing Details
28a8eae36c
fixed code formatting
a258wang requested changes 2022-03-28 00:01:23 -04:00
@ -0,0 +1,15 @@
.container {
display:flex;
Owner

There should be a space after the colon, ie. display: flex.

@snedadah Are you using VS Code? For VS Code users, we should have some kind of "format on save" set up for this repo, in order to maintain a consistent formatting style across everyone's code. (If you're using a different editor, then we might need to look into how we can apply our formatting rules there.)

There should be a space after the colon, ie. `display: flex`. @snedadah Are you using VS Code? For VS Code users, we should have some kind of "format on save" set up for this repo, in order to maintain a consistent formatting style across everyone's code. (If you're using a different editor, then we might need to look into how we can apply our formatting rules there.)
@ -0,0 +9,4 @@
padding: calc(20rem / 16);
padding-right: 0;
}
.heading {
Owner

Nitpick - there should be an empty line between lines 11 and 12.

Nitpick - there should be an empty line between lines 11 and 12.
@ -0,0 +17,4 @@
.container {
flex-direction: column;
}
.codey {
Owner

Nitpick again - there should be an empty line between lines 19 and 20.

Nitpick again - there should be an empty line between lines 19 and 20.
pages/404.tsx Outdated
@ -0,0 +5,4 @@
import { Title } from "@/components/Title";
import styles from "./404.module.css";
export default function Custom404() {
Owner

Another nitpick, there should probably be an empty line between lines 7 and 8 - we want to keep the imports and component visually separate. 😄

Another nitpick, there should probably be an empty line between lines 7 and 8 - we want to keep the imports and component visually separate. 😄
Owner

Just another small thing with padding: could we maybe sure the page has 60px of padding at the bottom on desktop, and 30px of padding at the bottom on mobile? The text/Codey still look like they're a little close to the bottom, in my opinion. (I believe these are the numbers used on the home page and some of the other pages as well.)

image
image

Just another small thing with padding: could we maybe sure the page has `60px` of padding at the bottom on desktop, and `30px` of padding at the bottom on mobile? The text/Codey still look like they're a little close to the bottom, in my opinion. (I believe these are the numbers used on the home page and some of the other pages as well.) ![image](/attachments/5b1734fa-d276-4e28-8210-fc96040eaf9a) ![image](/attachments/374a0585-2ed2-47da-bbea-eae2d625d3b4)
snedadah added 2 commits 2022-03-28 00:38:30 -04:00
Member

I think there's a prettier plugin for vim https://prettier.io/docs/en/vim.html (that's what we're using for formatting)

I think there's a prettier plugin for vim https://prettier.io/docs/en/vim.html (that's what we're using for formatting)
a258wang approved these changes 2022-03-28 01:38:06 -04:00
a258wang left a comment
Owner

Thanks for working on this, Shahan! We can continue to discuss what the text should say, and edit it in the future if we want to change it, but otherwise this PR should be good to merge!

Thanks for working on this, Shahan! We can continue to discuss what the text should say, and edit it in the future if we want to change it, but otherwise this PR should be good to merge!
snedadah merged commit c13b6a98f9 into main 2022-03-28 14:53:43 -04:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: www/www-new#410
No description provided.