Update sidebar.blade.php #3121
Update sidebar.blade.php #3121
Conversation
Use smarter logic to determine if menu item is active
|
BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly. Please keep in mind that:
Thank you! -- |
| @@ -58,11 +58,11 @@ function() { return $(this).attr('href') === full_url; } | |||
| // If not found, look for the link that starts with the url | |||
| if(!$curentPageLink.length > 0){ | |||
| $curentPageLink = $navLinks.filter( function() { | |||
| if ($(this).attr('href').startsWith(full_url)) { | |||
| if ($(this).attr('href').split('?')[0] === full_url) { | |||
There was a problem hiding this comment.
JavaScript's split always returns an array with at least one value, so even if there's no ? this still works as expected.
| return true; | ||
| } | ||
|
|
||
| if (full_url.startsWith($(this).attr('href'))) { | ||
| if (full_url.split('?')[0] === $(this).attr('href')) { |
|
The inspection completed: 5 updated code elements |
|
Thanks a lot of for the PR @joelmellon ! I agree that this bit of logic could use some work. It has corner cases like yours that are just not covered well. I agree with you that splitting by
The current implementation does leave out the use case you mentioned, though, and I hate that. Good news though - I have an out-of-the-box fix for it 🤣 Something that I've been eyeing for a while now but never got to do because new versions have been feature-packed already: we should probably make the route segment plural, not singular (maybe see Laravel-Backpack/Generators#63 for details). We only made it singular to match the Laravel convention at the time, even though we didn't agree to it. Now Laravel has made restful controllers plural, but we haven't, just to keep backwards-compatibility. But making the route segment plural, we should, in theory, fix this too, because the routes would be:
So for most corner cases like you mentioned, it should fix the problem. Right?! What are your thoughts on this @joelmellon ? Do you have a different programming solution to fix the problem above (sidebar items not active on other operations than List)? If not, I guess a quick fix for your project would be to implement plural route segments already. It's easy, just change the segment name both in Let me know what you think. |
|
@tabacitu "this renders the menu item inactive for all other operations" - I didn't even think about this (or notice it.) 🤦🏻♂️ I personally really like the plural URL segment convention, especially for index pages, but I prefer when they're singular for view, and edit, and create. I don't have a strong opinion though, and if Laravel's going in one of these directions, all the more reason to adopt it. Consistency is more important to me. I don't think it completely solves the problem, which you've pointed out, but it's a great leap in the short term. The issue I foresee if this logic doesn't change, is in the example I gave for users/usersettings. If the Again though, I do see your proposed solution as an improvement, and I'll be implementing it instead of the fix in the PR here for my own project. I don't have a ton of time to come up with something better, but if I do I'll let you know. Off the top of my head, I think there nav libraries out there that do this type of thing well, but I couldn't name any. In any case, the first step would be to nail the the definition (aka business rule) for what active means related to the URL. To me it's closely related to what comes before the GET vars, but if a user had GET vars in their nav link, they might expect the menu item to only be active if the exact URL matched, so it may not work for everyone. ‾\(ツ)/‾ |
|
Hmm you're right. There would still be uncovered cases, even with plural. So it still needs better a better JS solution. |
|
I made a small PR #3136 to enable the |
Damn it. You're right @joelmellon there's still that use case that wouldn't be covered... |
|
@joelmellon looks like we can't really consider this bug fixed. But we can't merge this either. Tough one. I'm going to close the PR though since we can't merge it. In the future, we'll:
But now that I think of it, if the end solution is a better convention (plural) I'm pretty sure another convention will fix the use case above too - using dashes. Right?! Freakin' dashes 🤣 Hear me out - if we use plural for route URLs AND use dashes to split words inside the route URLs we'll end up with:
Quite a simple solution, right?! 🤣 Why didn't we think of this before?! |
The current sidebar menu implementation isn't very smart when it comes to detecting which menu item should be "active" - it merely looks for the beginning of the URL in the href (and vice versa) which leads to multiple menu items being highlighted at once when they both begin with the same word, eg.
userandusersetting. See screenshot for my use case:
Above, I've got CRUD for models called `trail` and `trailhead` which means that when I'm on the trailhead page, both the Trailheads menu item is active (correctly) and the Trails menu item is active (incorrectly).This pull request implements improved logic to determine if menu item is active.
It could be made in better by also comparing the URL parameters (URL segment after the
?) but I think this is a significant improvement already.