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

Added active class to router link #294

Merged
merged 3 commits into from Mar 12, 2022
Merged

Added active class to router link #294

merged 3 commits into from Mar 12, 2022

Conversation

maccesch
Copy link
Contributor

@maccesch maccesch commented Mar 6, 2022

There should be a global way of setting the active class as well but I don't know how to properly do that. Please let me know what I can do.

@jkelleyrtp
Copy link
Member

I'll be honest, I didn't know that "active" was a thing :-)

I think the implementation makes sense - but does that also mean that all "Link" components subscribe to the current route? I guess that makes sense?

Anyways - I think you might've made these changes on a previous commit (I rebased the local router PR before/after a merge) and the method to get current location/path has changed a bit.

Otherwise - looks good and is the approach I would suggest!

@maccesch
Copy link
Contributor Author

maccesch commented Mar 7, 2022

Thanks for rebasing!
Yes, I’ve done work with Vue.js and Nuxt.js and there it works like this.
I guess if we really want to provide ‘active’ there’s no way around subscribing to the route, right?
Updating code…

@maccesch
Copy link
Contributor Author

maccesch commented Mar 9, 2022

Updated

Copy link
Member

@jkelleyrtp jkelleyrtp left a comment

Choose a reason for hiding this comment

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

Looks great! I think we might (later) update the router service to provide some of the parsing/validation logic internally for some extra caching. But - routes change so infrequently that we don't gain that much performance anyways.

Thanks for adding this!

edit: looks like rustfmt isn't very happy - a simple cargo fmt should do

@maccesch
Copy link
Contributor Author

@jkelleyrtp Hey, could you run the workflows again? Should work now

@jkelleyrtp jkelleyrtp merged commit 0fdd6d2 into DioxusLabs:master Mar 12, 2022
@jkelleyrtp
Copy link
Member

Thank you!!

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