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

Nav menu change with drop-downs #5750

Merged
merged 5 commits into from May 3, 2018

Conversation

Projects
None yet
4 participants
@marcrzzz
Copy link
Contributor

marcrzzz commented Apr 26, 2018

Problem

Really long side navigation menu that requires more scrolling to find content/links

Solution

Made some sections of navigation menu drop-downs, such as languages like JVM and python support, along with release notes

Result

Makes it easier to find links and necessary help for navigating the site
Changes can be seen at https://marcrzzz.github.io/pants/

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Apr 26, 2018

This looks great, thanks for putting it together!

The couple of unit tests which are failing on travis look related to this change - can you take a look at what's going on there? You should be able to reproduce by running: ./pants test pants-plugins/tests/python/internal_backend_test/sitegen

@cosmicexplorer
Copy link
Contributor

cosmicexplorer left a comment

I really like how this extends the functionality of docsite.json to support the dropdown functionality! Looking at src/docs/docsite.json and the required code to make this modification, it looks to me like this change was made harder than it could have been: for example, I tried to simplify the multiple if branches, and wasn't able to without destroying the rendered page. I think sometime in the future sitegen.py should be heavily edited, and we can definitely remove src/docs/docsite.json as well when that happens. For now, this is a great improvement, especially because it's extensible through docsite.json.

links=links,
dropdown=True,
heading=heading,
id=heading.replace(' ','-')))

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 27, 2018

Contributor

Generally, positional arguments are separated by a single space.

links=None,
dropdown=False,
heading=heading,
id=heading.replace(' ','-')))

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 27, 2018

Contributor

Arguments should probably be separated by a space, as above.

link = os.path.relpath(i + '.html', os.path.dirname(here))
links.append(dict(link=link,
text=precomputed.page[i].title,
here=(i == here)))

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 27, 2018

Contributor

This could probably be converted into:

for cur_page in pages:
  html_filename = '{}.html'.format(cur_page)
  page_is_here = cur_page == here
  if page_is_here:
    link = html_filename
  else:
    link = os.path.relpath(html_filename, os.path.dirname(here))
  links.append(dict(
    link=link,
    text=precomputed.page[cur_page].title,
    here=page_is_here))

The only thing I would say should almost definitely change is just using .format(), which is generally preferred to string concatenation in almost all cases. The rest just seemed to make it easier to follow, especially using '{}.html'.format(cur_page) instead of using here in the first branch of the if -- since i == here, then i + '.html' is the same as here + '.html', so it's easier to make that into a single statement. Let me know if I'm getting something wrong -- this kind of edit isn't something you need to add but I would recommend if it makes sense.

links = []
for i in pages:
if i == here:
link = here +'.html'

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 27, 2018

Contributor

Generally, binary operators like + are written with one space on either side.

link = os.path.relpath(i + '.html', os.path.dirname(here))
links.append(dict(link=link,
text=precomputed.page[i].title,
here=(i == here)))

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 27, 2018

Contributor

Same as above, this can be simplified a bit I think.

{{^here}}<a href="{{link}}">{{/here}}{{text}}{{^here}}</a>{{/here}}
{{#dropdown}}
<li class="toc-h{{depth}}">
<a class="sidebar-nav-heading" class= "toc-drop" data-toggle="collapse" href="#{{id}}" aria-expanded="false" aria-controls="{{id}}">{{heading}}<span class="caret"></span></a>

This comment has been minimized.

@baroquebobcat

baroquebobcat Apr 28, 2018

Contributor

It'd be nice if the menu could be left open if the current page is below the collapsible menu item. I think that'd require plumbing a value for aria-expanded here from the python side.

It'd be nice if nested collapsible usages could work too, but that might be a little tricky.

@@ -251,22 +251,46 @@ def generate_site_toc(config, precomputed, here):

def recurse(tree, depth_so_far):
for node in tree:
if 'heading' in node:
if 'heading' in node and 'pages' in node:

This comment has been minimized.

@baroquebobcat

baroquebobcat Apr 28, 2018

Contributor

I think the json would be clearer if collapsible sections had a different heading key word. Maybe 'collapsible_heading'?

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Thanks for the updates. I have another round of comments, but I think this is the last set.

"pages" : [
"python_readme",
"3rdparty_py"
]
},
{"pages" : ["go_readme"]},
{"heading": "Code & Doc Gen"},
{"collapsible_heading": "Code & Doc Gen"},
{"pages" : [

This comment has been minimized.

@baroquebobcat

baroquebobcat May 1, 2018

Contributor

I think that these two js objects be converted into one.

},
{"pages" : ["go_readme"]},
{"collapsible_heading": "Code & Doc Gen"},
{"pages" : [

This comment has been minimized.

@baroquebobcat

baroquebobcat May 1, 2018

Contributor

I think these two js objects should be made into one, like the other collapsibles.

else:
link = os.path.relpath(html_filename, os.path.dirname(here))
links.append(dict(link=link, text=precomputed.page[cur_page].title, here = page_is_here))
site_toc.append(dict(depth = depth_so_far, links = links, dropdown = True, heading = heading, id = heading.replace(' ','-'), open = collapse_open))

This comment has been minimized.

@baroquebobcat

baroquebobcat May 1, 2018

Contributor

nit: there shouldn't be whitespace around the =s in the kwargs in the dict calls here.
ie
call(x=y, z=a)
instead of
call(x = y, z = a)

else:
link = os.path.relpath(html_filename, os.path.dirname(here))
links.append(dict(link=link, text=precomputed.page[cur_page].title, here=page_is_here))
site_toc.append(dict(depth=depth_so_far, links=links, dropdown=False, heading=None, id=heading.replace(' ','-')))

This comment has been minimized.

@baroquebobcat

baroquebobcat May 1, 2018

Contributor

nit: add space after comma in replace(' ','-')

link=link,
text=precomputed.page[dst].title,
here=(dst == here)))
site_toc.append(dict(depth=depth_so_far, links=None, dropdown=False, heading=heading, id=heading.replace(' ','-')))

This comment has been minimized.

@baroquebobcat

baroquebobcat May 1, 2018

Contributor

nit: add space after comma in replace(' ','-')

{{/site_toc}}
""")
self.assertIn(u'DEPTH=1 LINK=subdir/page1.html TEXT=東京 is Tokyo', rendered)
self.assertIn('DEPTH=1 LINK=subdir/page2.html TEXT=Page 2: Electric Boogaloo', rendered)
self.assertIn(u'DEPTH=1 LINK=None HEADING=test', rendered)

This comment has been minimized.

@baroquebobcat

baroquebobcat May 1, 2018

Contributor

I'm not sure I understand why the assertions that use to be here were removed. I think they are still valuable because they were exercising the behavior around how links to pages are rendered. Maybe you could render both the heading and the text, and assert with DEPTH=... LINK=... HEADING=... TEXT=...?

Come to think of it, it would be good to have a collapsible_heading example here as well.

marcrzzz added some commits May 1, 2018

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Thanks for the updates!

@illicitonion illicitonion merged commit 6a1ac80 into pantsbuild:master May 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented May 3, 2018

Thanks for the great pull request! :) Merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment