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

fix: πŸ› treat isActive to case-insensitive #3657

Closed
wants to merge 1 commit into from

Conversation

Kyle-Ye
Copy link

@Kyle-Ye Kyle-Ye commented Oct 28, 2021

βœ… Closes: #3656

@Kyle-Ye Kyle-Ye marked this pull request as draft October 28, 2021 12:47
@Kyle-Ye Kyle-Ye marked this pull request as ready for review October 28, 2021 13:04
@Kyle-Ye
Copy link
Author

Kyle-Ye commented Oct 28, 2021

Maybe we need to add some tests for this new behavior, any suggestion where to write the new test?

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

This is not the correct patch as there is an option to make the path matching case sensitive and this should respect it.
This information should be available on the route records as regex.ignoreCase or something similar.
Can you also add a few unit tests for isIncludeRoute?

@Kyle-Ye
Copy link
Author

Kyle-Ye commented Oct 28, 2021

This is not the correct patch as there is an option to make the path matching case sensitive and this should respect it.
This information should be available on the route records as regex.ignoreCase or something similar.
Can you also add a few unit tests for isIncludeRoute?

Add the test and use regex.ignoreCase property. Any suggestion on how to safely get ignoreCase properly?

@@ -72,10 +72,11 @@ export default {
? createRoute(null, normalizeLocation(route.redirectedFrom), null, router)
: route

const ignoreCase = current.matched[0].regex.ignoreCase
Copy link
Author

Choose a reason for hiding this comment

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

Any suggestion on how to safely get ignoreCase properly? @posva

@Kyle-Ye Kyle-Ye requested a review from posva October 29, 2021 03:04
@Kyle-Ye
Copy link
Author

Kyle-Ye commented Nov 8, 2021

Hello @posva. Any suggestion on this PR?

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.

case-insensitive on isActive property
2 participants