fix mobile breadcrumbs #1652
fix mobile breadcrumbs #1652
Conversation
I broke this in cockroachdb#1477. Guess it wasn't "dead code."
|
This change is |
|
(Here's the original implementation: https://github.com/benesch/docs/blob/9f9757a8b825c9d6575a84e8d9e95798ac38a28a/_includes/sidebar.html#L54-L77) |
| })(); | ||
|
|
||
| function renderItems(items, tier) { | ||
| function renderItems(items, paths) { |
There was a problem hiding this comment.
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.
| }); | ||
| var subitems = renderItems(item.items, tier + 1); | ||
| var active = (urls.indexOf(location.pathname) !== -1); | ||
| if (active) { |
There was a problem hiding this comment.
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)?
| var subitems = renderItems(item.items, tier + 1); | ||
| var active = (urls.indexOf(location.pathname) !== -1); | ||
| if (active) { | ||
| $("#ch-breadcrumbs") |
There was a problem hiding this comment.
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?
|
All good points, but going to address in a follow-up PR to get this merged! |
Address review feedback from cockroachdb#1652.
Address review feedback from cockroachdb#1652.
Addresses review feedback from cockroachdb#1652.
Addresses review feedback from cockroachdb#1652.
|
Thank you, @benesch for the quick fix! 👍 |
|
Heh, sorry for breaking it in the first place.
…On Wed, Jul 5, 2017 at 2:12 PM, kuanluo ***@***.***> wrote:
Thank you, @benesch <https://github.com/benesch> for the quick fix! 👍
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1652 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA15IPlneY4EZm6QVB7lKR6ghi-a3Qzmks5sK9IogaJpZM4OLJxQ>
.
|
I broke this in #1477. Guess it wasn't "dead code." 🤦♂️