Skip to content
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

practice bootcamp adding 404 and 500 #198

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

krocodileteeth
Copy link

practice for the bootcamp that creates pages 404 and 500 for the registration website!

Closes #107

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

i did not test the 500 page. i did test the 404 page by going to a invalid URL.

  • Test A
  • Test B

Test Configuration:

  • Node.js version: 18.8
  • Python version: 3.8
  • Desktop/Mobile: Desktop
  • OS: Windows
  • Browser: Google Chrome

Checklist:

  • [X ] My code follows the style guidelines of this project
  • [ X] I have performed a self-review of my code
  • [X ] I have commented my code, particularly in hard-to-understand areas
  • [X ] I have made corresponding changes to the documentation
  • [ X] My changes generate no new warnings
  • [X ] I have added tests that prove my fix is effective or that my feature works
  • [ X] New and existing unit tests pass locally with my changes

@netlify
Copy link

netlify bot commented Sep 30, 2023

Deploy Preview for tartanhacks-registration ready!

Name Link
🔨 Latest commit 25bc046
🔍 Latest deploy log https://app.netlify.com/sites/tartanhacks-registration/deploys/6518852e88b0190008d4e368
😎 Deploy Preview https://deploy-preview-198--tartanhacks-registration.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pm3512 pm3512 self-requested a review September 30, 2023 20:30
@netlify
Copy link

netlify bot commented Sep 30, 2023

Deploy Preview for dev-tartanhacks-registration ready!

Name Link
🔨 Latest commit 25bc046
🔍 Latest deploy log https://app.netlify.com/sites/dev-tartanhacks-registration/deploys/6518852eadb56000083b370c
😎 Deploy Preview https://deploy-preview-198--dev-tartanhacks-registration.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pm3512
Copy link
Contributor

pm3512 commented Oct 1, 2023

Great job! I left a couple of comments about some very minor changes. Once those are fixed, this should be good to merge! VSCode has a tab on the left that shows the files you are about to stage or commit, this should help with fixing the line ending issue

import Menu from "src/components/menu/Menu"
import styles from "../styles/404.module.scss"

const ApplicationPage: NextPage = (): ReactElement => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is not actually used - it can be removed

import Menu from "src/components/menu/Menu"
import styles from "../styles/500.module.scss"

const ApplicationPage: NextPage = (): ReactElement => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same goes for this function - it is not used so it can be removed safely

@import "./variables.scss";
@import "./mixins.scss";

.background {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good! One thing - it seems like you use the same styles for your 404 and 500 pages. It is better to avoid duplication whenever possible - one solution would be to just use one file, eg errorPage.module.scss, that you would import in both 404.tsx and 500.tsx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add 404 and 500 pages
2 participants