-
Notifications
You must be signed in to change notification settings - Fork 124
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
Patch site navigation rendering of dropdown menus #297
Conversation
const nestedList = $(this).children().first(); | ||
const dropdownTitle = $(this).contents().first().text(); | ||
// Do not render dropdown menu for list items with <a> tag | ||
if ($(this).children('a').length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better if this was more explicit:
if ($(this).children('a').length > 0) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It follows Javascript
style where we use the truthy
values from an integer directly without doing an explicit comparison since it is guaranteed that the value is an integer >= 0.
nestedList.parent().replaceWith(formatSiteNav(nestedList.parent().html())); | ||
} | ||
// Found nested list, render dropdown menu | ||
} else if ($(this).children('ul').length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here:
} else if ($(this).children('ul').length > 0) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment above.
lib/Page.js
Outdated
@@ -131,14 +140,6 @@ function formatSiteNav(renderedSiteNav) { | |||
// Recursively format nested lists | |||
nestedList.replaceWith(formatSiteNav(nestedList.parent().html())); | |||
// Found element that is not a list, with nested items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be deleted now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well, some comments (see above)
8d91288
to
b3b1a33
Compare
Update
|
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Bug fix
Fixes #272
What is the rationale for this request?
What changes did you make? (Give an overview)
formatSiteNav
to account for styling tags in dropdown menu titlesdropdown-btn
CSS to align text to the left.Is there anything you'd like reviewers to focus on?
formatSiteNav
Testing instructions:
markbind serve
on thetest_site
folder.