Skip to content

Conversation

@patmellon
Copy link
Contributor

This PR updates the main navigation menu.

  • A link to the Mobile page has been added.
  • The Ecosystem dropdown has been added.
  • The Resources dropdown has been added.

Here's a preview of the updates: https://5d9f5672ff54b94fadd153d4--shiftlab-pytorch-docs.netlify.com/.

@JoelMarcey JoelMarcey requested review from brianjo and jlin27 October 15, 2019 21:34
@jlin27
Copy link

jlin27 commented Oct 15, 2019

I tried building this locally following these steps to install PR#54 for local testing and I'm getting the weird rendering:
image

My local steps to reproduce:

  1. Cloned pytorch/pytorch repo
  2. Removed github pytorch_sphinx_theme install from docs/requirements.txt
  3. Installed theme locally for PR:54 via https://fb.quip.com/HhcJAXRvbq9H
  4. make html and preview index.html

@jlin27
Copy link

jlin27 commented Oct 15, 2019

It looks like the extra text is what was meant to be in the drop down menu -
"...pre-trained models" and "Tools & Libraries" are from Ecosystem
and
"questions answered" and "About" are from Resources

Was it the addition of the ecosystem-dropdown and the resources-dropdown in the layout.html files that's causing this issue since the original nav in the docs page doesn't have a dropdown option?


<li>
<a href="{{ theme_variables.external_urls['features'] }}">Features</a>
<div class="ecosystem-dropdown">
Copy link

Choose a reason for hiding this comment

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

This section seems to be causing the render issues cause dropdown wasn't allowed before

Comment on lines +131 to +145
<div class="resources-dropdown">
<a id="resourcesDropdownButton" data-toggle="resources-dropdown">
Resources
</a>
<div class="resources-dropdown-menu">
<a class="nav-dropdown-item" href="{{ theme_variables.external_urls['resources'] }}"">
<span class=dropdown-title>Developer Resources</span>
<p>Find resources and get questions answered</p>
</a>
<a class="nav-dropdown-item" href="{{ theme_variables.external_urls['features'] }}">
<span class=dropdown-title>About</span>
<p>Learn about PyTorch’s features and capabilities</p>
</a>
</div>
</div>
Copy link

Choose a reason for hiding this comment

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

This section seems to be causing the render issues cause dropdown wasn't allowed before

@patmellon
Copy link
Contributor Author

@jlin27 I requested access to the quip link you shared so that I can see the steps you're following to build the docs with the theme.

From the screenshot you shared, it looks like the CSS I added in my PR is not being added to your local build--that's why the dropdown isn't appearing correctly. If you look at the generated theme.css file from the preview link I shared, you can see that the CSS is added correctly: https://5d9f5672ff54b94fadd153d4--shiftlab-pytorch-docs.netlify.com/_static/css/theme.css. (Search for "ecosystem-dropdown".) The theme.js file is updated as well: https://5d9f5672ff54b94fadd153d4--shiftlab-pytorch-docs.netlify.com/_static/js/theme.js. (Search for "mainMenuDropdown".) When building locally with my branch you should see these updated files appear in the pytorch_sphinx_theme/static folder.

If it's easier for me to include the updated theme.css and theme.js in my PR, I can do that. However, I didn't include those files since they are usually generated separately by your team.

@patmellon
Copy link
Contributor Author

@jlin27 I tried to build the docs following the steps you listed, and I can confirm that with your approach the updates I made won't appear correctly. Your steps assume that theme.css and theme.js are present in the PR. Since I didn't include those files in my PR, your build won't pick up the CSS changes I've made. Please let me know if you are changing the way you deploy the docs and if I should now include theme.css and theme.js in my PRs going forward.

@brianjo
Copy link
Contributor

brianjo commented Oct 17, 2019

@jlin27 I tried to build the docs following the steps you listed, and I can confirm that with your approach the updates I made won't appear correctly. Your steps assume that theme.css and theme.js are present in the PR. Since I didn't include those files in my PR, your build won't pick up the CSS changes I've made. Please let me know if you are changing the way you deploy the docs and if I should now include theme.css and theme.js in my PRs going forward.

Hey @Pat878, thanks for your work on this. Yes, it would be great if you could add all the updates to your PRs. We've always built docs using this install method, so getting everything into the new PRs would be best.

@patmellon
Copy link
Contributor Author

@brianjo Sounds good. I updated the PR to include the files.

@brianjo
Copy link
Contributor

brianjo commented Oct 17, 2019

Tested functionality locally. Looks good. Thanks @Pat878!
Screenshot 2019-10-17 08 14 31

@brianjo brianjo merged commit 9e76b28 into pytorch:master Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants