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

Add root url to href of page items #2671

Closed
wants to merge 1 commit into from
Closed

Add root url to href of page items #2671

wants to merge 1 commit into from

Conversation

idfunctor
Copy link
Contributor

Changes

Fixes #2258

Please describe the changes made in the pull request here.

The Link tag's href had only search params, which made a command click or an open in new window always end up at a 404.
Adding the root URL makes the link behave right.

Below you'll find a checklist. For each item on the list, check one option and delete the other.

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

@ukutaht
Copy link
Contributor

ukutaht commented Feb 14, 2023

Thanks @idfunctor !

Unfortunately it looks like we're on an older version of react-router. In v6, they've deprecated the useRouteMatch hook which you've used in this PR. I think the right direction is to upgrade to react-router@v6 first and then fix the bug with URL building.

@ukutaht ukutaht closed this Feb 14, 2023
@idfunctor
Copy link
Contributor Author

idfunctor commented Feb 14, 2023 •
edited

@ukutaht Yep, I had to rely on VSCode's auto-import to run into useRouteMatch as the "usual" react-router hooks weren't available, then I realised it's an older version.
I assumed though that it would make sense to fix the bug anyway as the package upgrade isn't blocking for a bug fix. But I guess it also makes sense to not add "more surface area" to the router upgrade effort. LMK if you want me to pick that up sometime between the next 2 weekends.

@idfunctor idfunctor mentioned this pull request Feb 19, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Top pages list: CMD+Click causes a 404
2 participants
@idfunctor @ukutaht