Website: Fix in page nav / left nav display bug#4199
Merged
Jahnp merged 12 commits intomicrosoft:masterfrom Mar 8, 2018
Merged
Website: Fix in page nav / left nav display bug#4199Jahnp merged 12 commits intomicrosoft:masterfrom
Jahnp merged 12 commits intomicrosoft:masterfrom
Conversation
This reverts commit 8a086bb.
mikewheaton
approved these changes
Mar 6, 2018
Jahnp
reviewed
Mar 7, 2018
|
|
||
| return _urlResolver.href === target; | ||
| path = getPathMinusLastHash(path); | ||
| if (path === target) { |
Member
There was a problem hiding this comment.
Maybe I'm misunderstanding the code here--but why are we evaluating if (path === target) twice: once after getting the href (new line 101), then again after stripping out the hash (line 106)? Can these be simplified?
Collaborator
Author
There was a problem hiding this comment.
Very fair point. It's in there twice because of the double hash situation - we only need the second if (path === target) for those cases. I refactored to do a hash count with one if (path === target) to make it more clear what the code is aiming for.
Collaborator
Author
|
@Jahnp are you good with the refactor? |
Member
|
@lynamemi thanks--this is much cleaner. Looks great to me! |
Jahnp
approved these changes
Mar 8, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request checklist
$ npm run changeDescription of changes
Left nav would disappear when you clicked on an in-page navigation link (like 'Best Practices' or 'Implementation').
By default, links in the nav are set to
display: none. The logic to check whether the link isActive or hasActiveChild (and set display to visible) was failing. Not sure why, because no recent changes have been made.I changed up how paths are generated for in-page nav to be more informative. They now will look like this:
#/components/activityitem#BestPractices
instead of:
#BestPractices
Then I reworked the logic for displaying the appropriate links.
Focus areas to test
(optional)