Skip to content

Peterson implement improvements in 404 page - #2962

Closed
peterson337 wants to merge 7 commits into
developmentfrom
Peterson_implement_improvements_in_404_page
Closed

Peterson implement improvements in 404 page#2962
peterson337 wants to merge 7 commits into
developmentfrom
Peterson_implement_improvements_in_404_page

Conversation

@peterson337

@peterson337 peterson337 commented Dec 18, 2024

Copy link
Copy Markdown
Contributor

This PR has conflicts that I couldn't resolve because the button at the bottom of the page was disabled. I needed to fix the bugs in this PR, so I decided to create a new PR with the bugs fixed and without the conflict error. After opening the new PR, I decided to close the old PR.

To test the new PR that implements the 404 page and does not have the bugs present in this PR, click this link: #3286

@peterson337 peterson337 added the do not review Do not review or look at code without full context label Dec 18, 2024
@netlify

netlify Bot commented Dec 18, 2024

Copy link
Copy Markdown

Deploy Preview for highestgoodnetwork-dev ready!

Name Link
🔨 Latest commit 8bfd455
🔍 Latest deploy log https://app.netlify.com/sites/highestgoodnetwork-dev/deploys/679aa4a60a48f7000817a693
😎 Deploy Preview https://deploy-preview-2962--highestgoodnetwork-dev.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.

@one-community one-community added High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible and removed do not review Do not review or look at code without full context labels Dec 22, 2024

@ManasaViswanadhapalli ManasaViswanadhapalli left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

mashpotato9
mashpotato9 previously approved these changes Dec 28, 2024

@mashpotato9 mashpotato9 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've reviewed this pr and everything worked as expected
Screenshot 2024-12-28 at 5 01 12 PM
Screenshot 2024-12-28 at 5 01 32 PM

vinayv-456
vinayv-456 previously approved these changes Jan 23, 2025

@vinayv-456 vinayv-456 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed code changes. "page not found" page is updated as mentioned
image

@honglinchen0524 honglinchen0524 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The error pages only shows up if the first path segment is invalid. If the first path segment is valid, it does not takes me to the error page(see first video).
Also it does not work for profile page(see second video).

PR2962.mp4
PR.mp4

@peterson337
peterson337 dismissed stale reviews from vinayv-456 and mashpotato9 via 8bfd455 January 29, 2025 21:59
@vivshar

vivshar commented Jan 31, 2025

Copy link
Copy Markdown

Fix works well when the 404 page is reached, however routes may need to be looked at as dashboard and timelog screens do not redirect to 404 page but teamlocations works.

PR.2962.mp4
PR.2962.2.mp4

@mikelambo7 mikelambo7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified that error page displays as expected both when logged in and not logged in
Screenshot 2025-02-04 at 2 49 43 PM
Screenshot 2025-02-04 at 2 50 06 PM

@yany960 yany960 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested error page for both logged-in and logged-out states, all redirects and modal working as expected.
image
image

@Peter-Nie2003 Peter-Nie2003 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The web work as expected, great job!

PR2962.mov

@k1guo k1guo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

#2922 I reviewed the code, it works as expected! thank you.

@AurHubertMax AurHubertMax left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested page where user is not logged in, along with volunteer, admin, and owner accounts. Page not found works as expected. Links in the page not found page also works as expected.

z.PR.2962.proof.1.mp4

@SSP24SCM16G SSP24SCM16G left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Issue: While http://localhost:3000/dashboard and http://localhost:3000/kjwhdg work as expected, http://localhost:3000/dashboard/wjhsdgc is also accessible, which is incorrect. sometimes image is not displayed.
Suggestion: Ensure proper route handling so that undefined subroutes under /dashboard return the correct error response instead of loading.
https://drive.google.com/file/d/1G-3OzDHtUiXPFS0kAORf-YujrLF3pKDD/view?usp=sharing

Uploading 2962.mov…

@rohith1667 rohith1667 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The feature works well as stated when user is logged in shows two links and when logged out shows only one link. There is a problem tough when checking a new non existing path the website is still loading instead of throwing an error. That should be checked properly!
Screenshot 2025-02-22 at 9 26 13 PM
Screenshot 2025-02-22 at 9 26 48 PM
Screenshot 2025-02-22 at 9 31 10 PM

@koushi1906

Copy link
Copy Markdown

I've tested the pull request, and the functionality is working as expected—logged-in users see two links, while logged-out users see only one.

However, I found an issue where the display image doesn't load correctly when there are multiple path separators (/) in the URL.

Additionally, when a user submits a report, the modal opens as expected, and the URL updates to http://localhost:3000/dashboard?openModalReport. However, even after closing the modal or submitting the report, if the page is reloaded, the modal opens again. This seems to be due to the query parameter still being present in the URL after the modal is closed.

Screenshot 2025-03-01 at 9 58 45 PM Screenshot 2025-03-01 at 9 59 11 PM

@HSP24SCM69K HSP24SCM69K left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed and tested the 404 Error Page improvements for both logged-in and logged-out users.

Functionality: All redirects are working as expected, with logged-in users correctly seeing options for the dashboard and suggestion modal, while logged-out users are redirected to the login page.
UI & User Experience: The design is clean, intuitive, and responsive, ensuring smooth navigation.
Performance: No errors were encountered, and all features function as intended.
Screenshot 2025-03-08 at 14 41 02
Screenshot 2025-03-08 at 14 41 12

@myeeli myeeli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have checked out the current PR and can confirm that the "Page Not Found" message is displayed correctly when the user is logged in and logged out, in both light mode and dark mode. Hence approving the PR.

PR 2962 - 1 PR 2962 - 2 PR 2962 - 3 PR 2962 - 4

@linh2020 linh2020 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have checked out the PR branch, installed dependencies, and cleared the cache. I logged in as both a regular and admin user and verified the error page behavior for non-existent routes. Everything works as expected — the 404 page displays the correct links based on login state, renders correctly in both light and dark modes, and no console errors appear. I also tested nested routes and verified that the modal behaves as expected.

2025-04-03 00_11_41-Login and 1 more page - Your Microsoft Edge - Microsoft​ Edge

2025-04-03 00_12_00-HGN APP and 1 more page - Your Microsoft Edge - Microsoft​ Edge

2025-04-03 00_13_25-Dashboard - Linh Admin and 1 more page - Your Microsoft Edge - Microsoft​ Edge

2025-04-03 00_13_40-HGN APP and 1 more page - Your Microsoft Edge - Microsoft​ Edge

2025-04-03 00_14_07-Login and 1 more page - Your Microsoft Edge - Microsoft​ Edge

2025-04-03 00_14_12-HGN APP and 1 more page - Your Microsoft Edge - Microsoft​ Edge

@peterson337 peterson337 closed this Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible

Projects

None yet

Development

Successfully merging this pull request may close these issues.