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

Revert react-router to v5 #509

Closed
wants to merge 2 commits into from
Closed

Revert react-router to v5 #509

wants to merge 2 commits into from

Conversation

fsmaia
Copy link
Contributor

@fsmaia fsmaia commented Dec 13, 2021

For bug fixes:

  • Add a reference to the original issue
  • Add a test and/or some kind of instructions to verify the fix

Fixes a regression introduced by #490.

Note: I'm unable to run the project locally, I need help testing.


For new features:

  • It's better to first discuss features proposal in Discussions or Slack
  • Describe the use-case in details
  • Ensure the solution is backwards-compatible
  • Test it. Submit screenshots / outputs / tests together with the PR.

--- Side note ---

Your contribution is very valuable, we really appreciate your time and effort!

Please note:

  • Adding a new code and / or feature complicates the maintenance
  • Most people disappear after contributing a feature, leaving the support to core maintainers

Given that, please don't be offended if your PR is rejected or significantly modified.

@fsmaia fsmaia requested a review from agoldis December 13, 2021 21:25
@fsmaia
Copy link
Contributor Author

fsmaia commented Dec 13, 2021

As I can't run locally, I can't figure out if useParams will work fine. Could you help me to test this?

Just start the project locally, create a project with slug, and try to access it.

@fsmaia
Copy link
Contributor Author

fsmaia commented Dec 14, 2021

Update: ran it locally (it was necessary to set Node version to v14), and it worked fine:
image

@ImanMahmoudinasab
Copy link
Contributor

ImanMahmoudinasab commented Dec 14, 2021

Hi Fernando!
My thoughts on the issue:
If we are going to use a value in the url it shouldn't include characters that can make issue if we put the value in the url. One solution could be not allowing the special characters, so we will be sure the name is url-friendly.
The other solution, which you mentioned on my PR, is encoding the value so users can use special characters if they have to. Yes it makes the url to not look so nice but it keeps url format valid.
Overall, I believe by looking at url and seeing / in the url user should have only one perception.

I don't think it is good idea to reveart the router to v5 to just allow having / in url as part of a param value and see it / instead of %3F.

@agoldis What 's your idea?

@agoldis
Copy link
Collaborator

agoldis commented Dec 14, 2021

I would go with encoded characters i.e. %3F

@ImanMahmoudinasab
Copy link
Contributor

I will create a pr and add the encoding and decoding logic to wherever we need it.
@fsmaia please feel free to create a pr if you have the chance to fix the issue before I start.

@fsmaia
Copy link
Contributor Author

fsmaia commented Dec 14, 2021

Ok, we may close this one then :)
Big thanks for the discussion!

@agoldis agoldis closed this Dec 15, 2021
@agoldis agoldis deleted the revert-react-router-to-v5 branch December 15, 2021 07:25
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.

None yet

3 participants