Skip to content

fix mobile breadcrumbs #1652

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 2, 2017
Merged

fix mobile breadcrumbs #1652

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions _includes/sidebar.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@

<ul id="mysidebar" class="nav {% if page.url == '/' or page.url == '/search.html' %}nav--home{% endif %}" style="display: none">
<div class="sidenav-arrow"><img src="{{ 'images/sidenav-arrow.svg' | relative_url }}" alt="Sidenav arrow"></div>
<div class="collapsed-header">
<div id="ch-breadcrumbs" class="collapsed-header__pre"></div>
<div id="ch-title"></div>
</div>
<li class="search-wrap">
<div class="search">
<span class="fa fa-search"></span>
Expand Down Expand Up @@ -89,32 +93,38 @@
return "stable";
})();

function renderItems(items, tier) {
function renderItems(items, paths) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I dunno how much we care about this for this code, but I might appreciate a comment showing the structure of the objects here since (I don't think) that's something you can easily figure out from this file.

if (!items || items.length == 0)
return $();

var lis = items.map(function (item) {
var urls = (item.urls || []).map(function (url) {
return baseUrl + url.replace("${VERSION}", pageVersion);
});
var subitems = renderItems(item.items, tier + 1);
var active = (urls.indexOf(location.pathname) !== -1);
if (active) {
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 not personally a huge fan of doing mutation within a map, maybe it would be clearer to change it to a forEach and append to a list at the end? Or even just extract this block into a function (if there's an easy way to do that)?

$("#ch-breadcrumbs")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I see what's happening here, you want to eventually overwrite with the value that occurs at the deepest point in the recursion? After figuring this out I think this logic is subtle enough that it could do with a comment explaining what's happening here, or maybe even pull this whole thing out into a separate function?

.html(paths.join("<div class=\"arrow-down arrow-down--pre\"></div>\n"));
$("#ch-title").html(item.title);
}
var subitems = renderItems(item.items, paths.concat(item.title));
var a = $("<a>")
.attr("href", urls[0] || "#")
.html(item.title);
if (subitems.length > 0) {
a.append(" ").append($("<div>").addClass("arrow-down"));
}
return $("<li>")
.addClass("tier-" + tier)
.toggleClass("active", urls.indexOf(location.pathname) !== -1)
.addClass("tier-" + (paths.length + 1))
.toggleClass("active", active)
.append(a)
.append(subitems);
});

return $("<ul>").append(lis);
}

document.write(renderItems({% include {{ page.sidebar_data }} %}, 1).html());
document.write(renderItems({% include {{ page.sidebar_data }} %}, []).html());
})()
</script>
</ul>
Expand Down