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.

Sign up for GitHub

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

Jump to bottom

BUG: 404 route not working on root route '/' #256

Closed
wants to merge 2 commits into from
Closed

BUG: 404 route not working on root route '/' #256

wants to merge 2 commits into from

Conversation

saivishwak
Copy link

@saivishwak saivishwak commented Sep 24, 2022 •
edited
Loading

Resolve #257

Description:

Redirect to page not found not working on root route

Usually everyone tends to type something on the root route ("/") to navigate to the next page. With the existing approach this feels a like a bad ux experience. And also later when the app needs to have different routes which are not related to data, they can go to their route like /settings/my_profile , /users/:userID. This will be useful when the app grows.

Checklist:

You should check all boxes before the PR is ready. If a box does not apply, check it to acknowledge it.

  • ISSUE NUMBER. You linked the issue number (Ex: Resolve #XXX).
  • PRE-COMMIT. You ran pre-commit on all commits, or else, you
    ran pre-commit run --all-files at the end.
  • USER CHANGES. The changes are added to CHANGELOG.md and the documentation, if they impact
    our users.
  • DEV CHANGES.
    • Update the documentation if this PR changes how to develop/launch on the app.
    • Update the README files and our wiki for any big design decisions, if relevant.
    • Add unit tests, docstrings, typing and comments for complex sections.

@gabegma
Copy link
Contributor

gabegma commented Sep 26, 2022

Thank you @saivishwak for adding issues to our repo and creating your first PR! This is really appreciated. @christyler3030, could you take a look?

@christyler3030
Copy link
Collaborator

christyler3030 commented Sep 26, 2022 •
edited
Loading

Hi @saivishwak thanks for raising this issue. We definitely need to handle these urls better but I think the fix will need to be deeper than this fix. For example, with this fix we still have the same undesired behaviour at the url: http://localhost:3005/app/asdfasdfasdfadf instead of http://localhost:3005/asdfasdfasdfadf

We will discuss some of the architectural concerns and will keep you informed. Thanks for the contribution!

@saivishwak
Copy link
Author

saivishwak commented Oct 5, 2022 •
edited
Loading

Hi @christyler3030 ,

Sure thanks a lot for your reply. Yes it does have same behavior at app/asdasfdasdf. My thought process was, its easier to get messy from home root ("/") so added another additional route.

@gabegma
Copy link
Contributor

gabegma commented Dec 7, 2022

I will close this PR, given the discussion. However, it would be great to find a lasting solution to this problem @christyler3030 @JosephMarinier. Thanks again @saivishwak for taking a stab at it!

@gabegma gabegma closed this Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christyler3030 christyler3030 Awaiting requested review from christyler3030

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Redirect to page not found not working on root route
3 participants
@saivishwak @gabegma @christyler3030