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?
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).
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
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?
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.
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.
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.)
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.)
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.)
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!
n3parikh
j285he
a258wang
Closes #282
demo: https://csclub.uwaterloo.ca/~a3thakra/csc/404page/404/
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?
A few minor comments
.container{Add space between
rand{(and also for thecodeyclass).container{display:flex;Use 2 spaces for indentation rather than tab.
return (<div className={styles.container}><div className={styles.text}><Title>Error 404</Title>I think title should be a bit more descriptive. Maybe something like "404: Page Not Found"?
<div className={styles.text}><Title>Error 404</Title><h2>Error 404</h2><h1>We couldn't find the page you're looking for!</h1>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).<h1>We couldn't find the page you're looking for!</h1><p>We're working on it, but in the meantime,<Link href="/"> click here to go back to the main page.</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
Some comments:
.container {display:flex;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 {Nitpick - there should be an empty line between lines 11 and 12.
.container {flex-direction: column;}.codey {Nitpick again - there should be an empty line between lines 19 and 20.
import { Title } from "@/components/Title";import styles from "./404.module.css";export default function Custom404() {Another nitpick, there should probably be an empty line between lines 7 and 8 - we want to keep the imports and component visually separate. 😄
Just another small thing with padding: could we maybe sure the page has
60pxof padding at the bottom on desktop, and30pxof 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.)I think there's a prettier plugin for vim https://prettier.io/docs/en/vim.html (that's what we're using for formatting)
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!
c13b6a98f9into main 1 year agoReviewers
c13b6a98f9.