Added custom 404 page #410

Merged
snedadah merged 12 commits from 404page into main 1 year ago
Owner
Closes #282 demo: https://csclub.uwaterloo.ca/~a3thakra/csc/404page/404/
snedadah added 5 commits 1 year ago
8486160c2a added 404 page and styling
b723a0cde0 fixed formatting of 404 text
3b02b0b5db escaped apostroph's
7192fe6681 updated codey
6af42d77b7 Empty Commit
snedadah requested review from n3parikh 1 year ago
snedadah requested review from j285he 1 year ago
snedadah requested review from a258wang 1 year ago
Poster
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 1 year ago
n3parikh left a comment
Owner

A few minor comments

A few minor comments
.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
.container{
display:flex;
Owner

Use 2 spaces for indentation rather than tab.

Use 2 spaces for indentation rather than tab.
pages/404.tsx Outdated
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
<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 1 year ago
pages/404.tsx Outdated
<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>
Collaborator

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 1 year ago
fe4b699686 made slight adjustments to 404 page
snedadah added 1 commit 1 year ago
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 1 year ago
efcdd7ef52 trimmed 404 codey graphic
snedadah added 1 commit 1 year ago
54c4dcd2ed adjusted spacing of 404 page
snedadah added 1 commit 1 year ago
28a8eae36c fixed code formatting
a258wang requested changes 1 year ago
.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.)
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.
.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
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 1 year ago
Collaborator

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 1 year ago
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 1 year ago
snedadah referenced this issue from a commit 1 year ago

Reviewers

n3parikh was requested for review 1 year ago
j285he approved these changes 1 year ago
a258wang approved these changes 1 year ago
continuous-integration/drone/push Build is passing
The pull request has been merged as c13b6a98f9.
Sign in to join this conversation.
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/www-new#410
Loading…
There is no content yet.