Skip to content
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

Fly-out menu broken from MkDocs projects #4549

Closed
marcelstoer opened this issue Aug 21, 2018 · 12 comments
Closed

Fly-out menu broken from MkDocs projects #4549

marcelstoer opened this issue Aug 21, 2018 · 12 comments
Labels
Bug A bug

Comments

@marcelstoer
Copy link
Contributor

This is a follow-up to #4314 (comment)

Details

Expected Result

  • The usual fly-out menu bottom left

Actual Result

  • No fly-out menu, just a GitHub link and previous/next links
  • If my own analysis is correct than this broke when you moved the default MkDocs version to >= 0.16 (currently 0.17.3).

Note that for https://mkdocs.readthedocs.io/en/stable/ the behavior is different for reasons unknown to me. There you get a somewhat crippled fly-out bottom-right once you scroll all the way to the end of a page.

@stsewd
Copy link
Member

stsewd commented Aug 21, 2018

Note that for https://mkdocs.readthedocs.io/en/stable/ the behavior is different for reasons unknown to me. There you get a somewhat crippled fly-out bottom-right once you scroll all the way to the end of a page.

I guess that is because of #4448

Not sure about the problem with your docs :/

@marcelstoer
Copy link
Contributor Author

marcelstoer commented Aug 21, 2018

#4448 isn't exactly the same as it mentions changes for MkDocs 1.0. The fly-out broke with your 0.15.x to 0.17.3 upgrade. It means it's currently broken for all MkDocs projects, I assume, that don't explicitly downgrade MkDocs via requirements.txt or so.

I guess this issues is missing the MkDocs label. What does the Needed: replication really mean? I can reproduce the bug.

@stsewd
Copy link
Member

stsewd commented Aug 21, 2018

We don't have the Mkdocs label anymore, Needed replication means that someone else can replicate the issue. mkdocs is using the install option, so they are using that version to build the docs (1.0.1)

@stsewd
Copy link
Member

stsewd commented Aug 21, 2018

Yep, I can confirm that mkdocs/mkdocs#1571

@stsewd stsewd removed the Needed: replication Bug replication is required label Aug 21, 2018
@stsewd
Copy link
Member

stsewd commented Aug 21, 2018

I think this bug was introduced in https://github.com/rtfd/readthedocs.org/pull/4499/files, I'm investigating

@davidfischer
Copy link
Contributor

Note that for https://mkdocs.readthedocs.io/en/stable/ the behavior is different for reasons unknown to me.

They are building with MkDocs 1.0+. I think the issues are related to that. I outlined some of the issues we have with MkDocs 1.0 here: #4448

@stsewd
Copy link
Member

stsewd commented Aug 21, 2018

I found that you are using https://github.com/nodemcu/nodemcu-firmware/blob/fe40323ec49a0169ab2ca9a8722f623665b5e5ec/mkdocs.yml#L5-L6

Which I think should just work with mkdocs 0.17 :/ but the for some reason the rtd templates doesn't override the theme

@davidfischer
Copy link
Contributor

I am going to move #4448 up my priority list and try to get a PR as soon as possible. There have been some significant compatibility problems between Read the Docs and mkdocs and a lot of them stem from different users using different versions of mkdocs and the difficulty in testing that. It also doesn't help that the mkdocs Read the Docs theme is outside of our control (as opposed to the Sphinx Read the Docs theme which we do control) and mkdocs made a few changes to it that broke some of what we were doing.

I think the solution is to:

  • Make as few changes to mkdocs.yml as possible
  • Don't pass the --theme option when executing mkdocs
  • Don't override mkdocs's readthedocs theme with theme_dir or custom_dir
  • Use the regular lower right corner badge-only flyout menu like we do on non-Read the Docs themes (eg. as on alabaster).

Does that sound good?

@davidfischer
Copy link
Contributor

I figured out exactly what the problem is and why our template overrides weren't taking effect (which in turn broke the flyout menu). The problem is that the --theme option changed slightly with 0.17. Starting in 0.17, it seems like passing the --theme option to mkdocs (as Read the Docs currently does) causes mkdocs to ignore template overrides. Strangely, this is only true if you use the new nested theme directive for theme.custom_dir introduced in 0.17. If you use the deprecated theme_dir then template overrides take effect even with --theme.

I am going to work toward a solution along the lines of my comment above

@davidfischer
Copy link
Contributor

I have a PR along the lines of what I discussed above: #4556 Any feedback on that PR is welcome.

@marcelstoer
Copy link
Contributor Author

This being closed doesn't mean that the fix is already in production i.e. live, is it? Reason for asking: I'm still not seeing any version flyout/flyout-replacement on https://nodemcu.readthedocs.io/en/latest/en/support/.

@stsewd
Copy link
Member

stsewd commented Sep 17, 2018

@marcelstoer yes, this isn't deployed yet. All deployed code is merged in rel branch (from master)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug
Projects
None yet
Development

No branches or pull requests

4 participants