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

docs(menu): Fix selection highlight in menu service example #1660

Closed
wants to merge 1 commit into from
Closed

docs(menu): Fix selection highlight in menu service example #1660

wants to merge 1 commit into from

Conversation

miqh
Copy link

@miqh miqh commented Jun 29, 2019


Hello,

This is a very minor documentation fix.

I just noticed the menu service example doesn't correctly highlight one of the last items in the menu, presumably due to how the menu item is configured to match (i.e. full path by default).

Copy link
Contributor

@yggg yggg left a comment

Choose a reason for hiding this comment

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

Thanks for contribution @miqh 👍
Please see comment below.

@@ -39,8 +39,6 @@ export const MENU_ITEMS = [
title: 'Commercial',
link: '/example/menu/menu-service.component/3/3/2',
icon: 'grid-outline',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid there is no issue with configuration. Looks like the fix (#1034) aimed to include fragments into comparison doesn't work.

See here:

? isFragmentEqual(this.location.path(), item.fragment)

There are calls for isFragmentEqual and isFragmentContain functions which expecting path with the fragment. But this.location.path() doesn't return fragment unless you provide true as a parameter to the path function.

So a proper fix for the issue would be adding true as a parameter for this.location.path calls on lines 387 and 388.

Another thing to mention, there is plenty of tests for path and fragment comparison here and they all are passing. As turns out, SpyLocation used in unit tests as a mock for Location has no parameters for path method and always returns path with the fragment. I reported it here.
Until it's fixed in Angular it would be great to work around it somehow in Nebular tests.
I think we can extend SpyLocation, override path method to respect includeHash parameter and provide our extended class during TextBed configuration.
You can use helper functions from url-matching-helpers.ts.

What do you think? 🙂
Feel free to ping me here if you need any assistance.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I see, my mistake. I obviously didn't probe far enough into the underlying cause.

@miqh miqh closed this Jun 29, 2019
@yggg yggg mentioned this pull request Jul 12, 2019
2 tasks
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.

2 participants