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 '.' escaping in current() #296

Merged
merged 1 commit into from Jun 1, 2020
Merged

Fix '.' escaping in current() #296

merged 1 commit into from Jun 1, 2020

Conversation

d13r
Copy link
Contributor

@d13r d13r commented May 30, 2020

No description provided.

@bakerkretzmar
Copy link
Collaborator

@davejamesmiller can you explain what this does and why it's necessary? Thanks.

@bakerkretzmar bakerkretzmar added the needs more info Waiting for more information label May 31, 2020
@d13r
Copy link
Contributor Author

d13r commented May 31, 2020

@bakerkretzmar

From the docs:

To check whether you are currently on a specific route, you can pass the route name to current():
route().current('events.index');
// returns true

The . is not currently being escaped correctly, so events-index would also match.

Specifically in my case, it means hosting.* is compiled to ^hosting..*$ instead of hosting\..*$, so current('hosting.*') returns true for the route hosting-contacts.index as well as hosting.index and the wrong menu item is highlighted.

@d13r
Copy link
Contributor Author

d13r commented May 31, 2020

It looks like there was originally an attempt to escape it, but it didn't work correctly because '\.' === '.' in JS, and then it was auto-formatted into .replace('.', '.') which is more clearly a no-op.

@bakerkretzmar bakerkretzmar changed the base branch from master to 0.9.x June 1, 2020 00:17
@bakerkretzmar
Copy link
Collaborator

Wow, '.' === '\.' is truly messed up....

Thanks a lot for clarifying, and for the fix! Gonna try to merge into the 0.9.x branch so we can release it as a patch.

@bakerkretzmar bakerkretzmar removed the needs more info Waiting for more information label Jun 1, 2020
@bakerkretzmar
Copy link
Collaborator

Nope I'm not, that got very messy in git very quickly 😂 will do a patch release separately.

@bakerkretzmar bakerkretzmar changed the base branch from 0.9.x to master June 1, 2020 00:30
@bakerkretzmar bakerkretzmar merged commit 1eccdf2 into tighten:master Jun 1, 2020
@d13r
Copy link
Contributor Author

d13r commented Jun 1, 2020

Thanks @bakerkretzmar 😃

@d13r d13r deleted the patch-1 branch August 13, 2020 15:00
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

2 participants