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

Implement 404 page #906

Conversation

MehulKChaudhari
Copy link

@MehulKChaudhari MehulKChaudhari commented Feb 15, 2024

Fixes: #884

Desktop:
image

Mobile:
image

@MehulKChaudhari MehulKChaudhari changed the base branch from master to website-redesign February 15, 2024 20:47
Copy link

netlify bot commented Feb 15, 2024

Deploy Preview for ember-api-docs ready!

Name Link
🔨 Latest commit 96fbf4d
🔍 Latest deploy log https://app.netlify.com/sites/ember-api-docs/deploys/65d773aaaaa5e4000855a9a8
😎 Deploy Preview https://deploy-preview-906--ember-api-docs.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.

@MehulKChaudhari
Copy link
Author

Hey @MinThaMie, Please have a look at this PR? I have made changes keeping website-redesign as base branch.

@MehulKChaudhari MehulKChaudhari changed the title Issue 884/implement 404 page Implement 404 page Feb 15, 2024
@evoactivity
Copy link

Could I suggest something more like this

image image
  1. Add padding to the .whoops class, my example screenshots are 4rem on desktop and 2rem on mobile
  2. remove flex-direction: column; on desktop
  3. reduce size of tomster to around 240px on desktop

@MinThaMie
Copy link
Contributor

I wholeheartedly agree with @evoactivity :) If you implement those changes I'll approve :)

@MehulKChaudhari
Copy link
Author

I wholeheartedly agree with @evoactivity :) If you implement those changes I'll approve :)

Made the changes, but Instead of 2rem padding in mobile I have used --spacing-3 variable which is 1.5rem does that work or we should add hard coded 2rem value?
@MinThaMie @evoactivity

@MinThaMie
Copy link
Contributor

I'm happy with the --spacing-3

Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

one last thing and it should be good to go 👍

tests/unit/routes/not-found-test.js Outdated Show resolved Hide resolved
@MinThaMie MinThaMie dismissed mansona’s stale review February 22, 2024 16:37

Cause it was fixed :)

@MinThaMie MinThaMie merged commit 1e643e7 into ember-learn:website-redesign Feb 22, 2024
4 checks passed
@MinThaMie
Copy link
Contributor

Thank you very much @MehulKChaudhari

@MehulKChaudhari
Copy link
Author

Thank you very much @MehulKChaudhari

Thank you, Please let me know if there is any task that I can pick up.

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

Successfully merging this pull request may close these issues.

Implement 404
4 participants