-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Updates and simplification for mkdocs #4556
Conversation
@@ -7,6 +7,9 @@ | |||
display: block; | |||
|
|||
bottom: 50px; | |||
|
|||
/* Workaround for mkdocs which set a specific height for this element */ | |||
height: auto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the bug seen here which is currently visible on https://mkdocs.readthedocs.io/en/stable/
'%s/readthedocs/templates/mkdocs/readthedocs' % settings.SITE_ROOT | ||
) | ||
# The default theme for mkdocs is the 'mkdocs' theme | ||
DEFAULT_THEME_NAME = 'mkdocs' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not default to the rtd theme when users don't have a theme
or theme.name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting with mkdocs>=1.0, theme is required. Also, this is another source of confusion where Read the Docs does something different than the default MkDocs functionality. I believe we shouldn't be changing users' intents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that makes sense, I wonder if this is good for old projects, all of them were building with the rtd theme, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to keep the same behavior outside than inside RTD.
I want to echo Santos' question: what would happen with old projects that weren't specifying the them? Will they start building with the mkdocs
theme? If so, I suppose we should do something to avoid that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming they target mkdocs<1.0
and don't specify a theme, they will start to build with the mkdocs
theme. This would be the same behavior as if they built locally. Starting with mkdocs>=1.0
, they will fail to build with an error because theme is required.
Frankly, I think switching them to the mkdocs
theme is the correct behavior as then their project builds with the same theme inside and outside RTD. In general, however, I suspect not a lot of projects do not specify a theme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am -1 on this particular change, I think projects should keep the readthedocs theme if that is what they have been building their projects with. We can use a date-based feature flag if necessary, to ensure that projects before the feature flag get the readthedocs
theme.
If the user hasn't changed to a custom theme, I don't think we can accurately determine whether they want the default mkdocs theme or our theme, so i'm more inclined to just keep our theme for these users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put some thoughts here about this. I moved the discussion a bit since it references multiple comments.
I built a small project that lets you switch between different themes and versions of mkdocs. It is probably useful for verification but also for illustrating some of the issues highlighted above. https://rtd-mkdocs-test-project.readthedocs.io/
Edit: This PR is supposed to fix these issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ❤️ these changes.
I'm not sure about the JS ones because I don't know that code, but they look good to me.
I just left a question that we should consider for old project that are not defining any theme. For those projects, we should keep readthedocs
theme, I suppose.
One thing I would like to add either to this PR or another one is to recommend that authors using mkdocs on Read the Docs have a requirements file that pins mkdocs to a major version. We don't have a lot of docs on mkdocs, but do you think https://docs.readthedocs.io/en/latest/builds.html is a good place? |
https://docs.readthedocs.io/en/latest/getting_started.html looks like a good place, maybe another PR making the suggestion for both, sphinx and mkdocs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The less we mess with mkdocs, probably the better, so that we have the most compatibility. I don't think we're indexing search from mkdocs, so I don't know how much value we're really providing for projects beyond just being a dumb host, but that's probably fine in terms of compatibility.
Hosting multiple concurrent versions is something but I agree. |
I added a note about pinning versions in #4562. |
Is it pure coincidence that over at MkDocs a PR emerged recently to align their version of the RTD theme with upstream again: mkdocs/mkdocs#1594 |
Makes me wonder...was is/was the motivation to support MkDocs as a documentation type then? Would things be different if the Sphinx + CommonMark combination had existed when you started? |
I believe it is coincidence. I haven't spoken with them about it. |
While I'm not super tied to requiring our theme, removing default of our theme for mkdocs projects like this does feel like we're stepping further away from mkdocs support and into straight dumb host territory. I think this will just make me even less inclined to support mkdocs going forward. At least now, our branding is apparent and we have added some functionality. I really don't know what the middle ground is though. Obviously the user experience now is not really acceptable either. I guess I'd be mostly 👍 on removing our theme as the default under the assumption that we are returning to this to 1) resolve the theme support issues, 2) re-enable our theme as default, and 3) get mkdocs closer to feature parity. Otherwise, without someone from the mkdocs community helping to guide a mkdocs integration, we aren't creating a good user experience here -- and it's costing a good deal of effort still. |
I'm not involved with either community; just an active user of both MkDocs and RTD (creating issues and the odd small PR here and there). I appreciate the effort @davidfischer has been putting into this but I share @agjohnson's concerns. I feel that RTD should have a vital interest that all projects it hosts use the default theme.
How about just supporting 1.x going forward?
As a user I would be very happy to see open or behind-the-scenes collaboration between RTD and @waylan. A smooth and seamless integration would/should benefit both projects I would assume. |
Sadly we still need to keep building projects < 1.x, as they are already has been built by rtd. Breaking those projects is a bad user experience. |
I agree with this, but maybe we can add this also to our deprecation list for next year together with APIv1 and others. What do you think? I'm assuming that this will simplify our code and we could remove some "guessing" that we have on it. |
Firstly, 1.x literally just came out. Deprecating pre-1.x in 4 months seems fast. Secondly, how would you actually enforce this? People can just put Overall, I think we need to firm up how we are going to handle builder requirements. Currently people can specify any version of sphinx or mkdocs they want and Read the Docs is just expected to work. Perhaps based on people's |
@agjohnson I want to dig in on this statement a bit. I completely understand that the goal of Read the Docs is not to be a dumb host but I also want to make sure we are actually adding value when we say we are.
To me, I think this means we as the RTD maintainers need to do a better job setting out the vision for MkDocs (and Sphinx and other future docs builders) and how RTD aims to add value over a simple docs solution like building docs in Travis and hosting with GitHub Pages. Figuring this out is how we expand from being "the destination for Python docs" to "the destination for OSS docs".
|
As a member of the MkDocs team, I agree. In fact, because RTD has traditionally ignored the theme setting and forced their own theme, we have mostly ignored the needs of RTD when developing MkDocs. However, if RTD is willing to honor users wishes (for themes, plugins etc), then we may be more willing to consider the needs of RTD. For example, RTD needs to provide a way for users to be able to specify third party themes and/or plugins. This requires that the Python package first be installed, and then that the user include it in their MkDocs config file. It may be posable, but I have not seem any documentation on how to specify Python packages that are build dependencies on a project-by-project basis. Therefore. to most users, only the built-in themes and plugins are available. I'd like to see clear, documented support for third party themes and plugins, otherwise, trying to host MkDocs projects on RTD is a waste of users time. Currently, the RTD theme included with MkDocs includes a partial template versions.html, which gets included in every page and, by default, does not display any "version" information. Of course, RTD could easily provide their own In other words, this:
|
This is currently possible and there are users doing this. RTD supports including a requirements file and then users can include whatever Python packages they want including plugins and themes. However, I'll agree that documented examples of third party theme and plugin use would be helpful. There is a bug with themes (which this PR fixes) where depending on the mkdocs version the theme may be overridden with the RTD theme.
I don't know when this last was and RTD has definitely gone through time periods of being overwhelmed. However, our issue tracker is now quite active and if you have questions or want answers, don't hesitate to open an issue. I'll do the same on yours. Let's keep an open line of communication. |
Apologies in advance, many thoughts incoming
The main value in maintaining a blessed Sphinx or mkdocs theme is that we can provide the reader user an experience that is consistent, expected, and highly usable. Cross device support is lacking for most documentation -- this was the main force behind developing a Sphinx theme in the first place. I have more thoughts on where our theme and support for themes are headed, but that's another discussion. It is however in the direction of better generic support for multiple themes. But for now, we've tied our user experience to our lone theme and it's readership focus.
We also force our theme on Sphinx projects by default, unless you explicitly specify a theme. I think the the behavior with mkdocs should match what we do with Sphinx, which isn't what we're doing ATM.
For some history here, we had to force our theme as at the time, it was the only way for us to make our search backend integration work (and I believe the only way for us to get mkdocs to work at all). d0ugal was helping to maintain the integration for a while, but also lost the bandwidth to support this. Since then, our search integration is now 100% broken (#1088) and that's bad -- we need to get back to supporting our indexing in mkdocs to provide a consistent search. Forcing our theme is certainly not a requirement, but the extension points that we need to provide some parity here didn't/don't exist in mkdocs. This really requires someone from the mkdocs community to help maintain, we can only provide second class support with hacks otherwise.
This is a long list, but I think we can overlook the disparity between authorship features if at least mkdocs has a somewhat similar readership user experience. Not breaking search UX is probably the biggest point here, but I don't know what it takes to get back to in-doc search on our backend. We were historically relying on some mkdocs code that was removed a few versions ago. We need a json output similar to Sphinx's to feed our index (regardless of theme) and we need a method of forcing our search backend on the built pages. Future plans for search involve more in-doc search, so this divide in features will only become more apparent.
Agreed. We can certainly layout more issues on Mkdocs if there is someone willing to take the work on. Without any interaction with the mkdocs community, it's mostly just stale issues. We need buy-in from the mkdocs community to support an integration properly, as we're spread too thin to take on support of a mkdocs ecosystem on top of Sphinx. Honestly, I'm already way more inclined to spend our time developing Sphinx core/extensions -- it's hard to justify the cost of building up another toolchain when the features we rely on already exist in Sphinx.
From an architectural/engineering standpoint, I definitely agree. Extensions are probably an more reasonable way for us to bend mkdocs to our will. Having a similar set of authoring features can certainly be a community effort. 👍 Unfortunately, we don't have the bandwidth to develop mkdocs tooling into the tooling that Sphinx is, especially when we're already neglecting efforts/opportunity to really improve Sphinx authorship features.
I'm not familiar with Long term, the vision for our version menu that I've been harping on for a while, is to make the footer return JSON, not HTML, so that the theme can do whatever it wants with the data. This might be our standard flyout, it might be a theme-specific flyout (like sphinx_rtd_theme). We need a strong API there though, in order to justify allowing themes to produce their own flyout experience. Also, a stronger API won't remove the need for RTD to require more control over mkdocs. Extensions are probably still fine here. Going forward
|
What we on the MkDocs team need from RTD is a clear documented list of those extension points. For example, we removed the JSON output because we were of the impression that it was unused (any internal use case had been replaced) and we considered it a waste of our resources to maintain. If we had a definitive list of things RTD needs, we could avoid that in the future.
I think having the version data in JSON makes a lot of sense. I'm not exactly sure how that would look, but I like the general idea. FYI, in mkdocs/mkdocs#1594 we have a PR which updates MkDocs RTD theme to better match the Sphinx RTD theme in look, behavior and functionality. It hasn't been finalized yet, but I expect it to be part of the MkDocs 1.1 release. Any feedback is certainly welcome. |
@agjohnson I'm not super-familiar with how RTD works as I've never used it, but
In If this sounds roughly-similar to RTD, I'm happy to make changes to the |
Versions on RTD are tied to branches/tags, so no metadata is required on RTD. Our flyout API will just put this information into docs based on versions that are active at the moment, so it's currently working and probably not important to get addition context data into mkdocs themes unless we're doing something special. |
@agjohnson from the MkDocs side, we don't care where you get your data from. We care about what format it is presented in to the theme. Document that and we'll consider building support for it into all our themes. |
@waylan in due time, sure. The problem now is we're not sure what needs to change between mkdocs and RTD -- hence this PR that brings mkdocs to a mostly vanilla experience. We're focusing on this PR first. |
@agjohnson Just to clarify, The important part of this is that it means the list of versions is dynamic. As mentioned above, In any case, I'm happy to help converge on a standard here, but I'm also equally happy with letting |
This PR is a little off the rails but I created #4588 to discuss more vision around MkDocs/RTD and possibly |
We released these changes to production today as part of our release. @marcelstoer, everything looks good on NodeMCU. Let me know if you see otherwise. |
I was notified by GitPunch, thanks. So, you were the one who triggered a doc build just 15min ago? I also did that as soon as I learned about the release 😉 You're right, everything looks ok - except that I don't see any fly-out at all, neither bottom left nor bottom right. |
Yep, spot on. I also noticed that the view/edit On GitHub links are still broken but I don't know if they're provided like that from MkDocs. |
I'm not super familiar with it to be honest but I don't think RTD is doing anything special. |
Oh, I think I have a PR to fix that (if you are asking for the link on the flyout menu) |
Here it is #3525 |
Oh ya, the github link in the flyout menu (provided by us) is broken. |
Historically, we were using `readthedocs` as default theme for MkDocs but in #4556 we decided to change it to get the same behavior when building locally than in Read the Docs. This commit adds a Feature flag to keep having the old behavior for some particular projects so we can add these project and do not break their documentation (change the theme without asking/reason).
* Feature flag to make `readthedocs` theme default on MkDocs docs Historically, we were using `readthedocs` as default theme for MkDocs but in #4556 we decided to change it to get the same behavior when building locally than in Read the Docs. This commit adds a Feature flag to keep having the old behavior for some particular projects so we can add these project and do not break their documentation (change the theme without asking/reason). * Typo
Historically, we were using `readthedocs` as default theme for MkDocs but in #4556 we decided to change it to get the same behavior when building locally than in Read the Docs. This commit adds a Feature flag to keep having the old behavior for some particular projects so we can add these project and do not break their documentation (change the theme without asking/reason).
This PR simplifies the mkdocs builder and tries to have maximum compatibility between mkdocs versions.
Changes
--theme readthedocs
option to mkdocs. Starting inmkdocs>=1.0
, this overrides theme settings inmkdocs.yml
and it can overridemkdocs.yml
depending on some options in 0.17 (see this comment for details). This makes the default theme on MkDocs the "mkdocs" theme rather than the readthedocs theme. People who want the readthedocs theme will need to specify that in theirmkdocs.yml
. This is the default functionality of mkdocs outside of Read the Docs.Screenshot
Notice the flyout menu in the lower right rather than overriding the mkdocs RTD theme.
Fixes
Fixes #4314
Fixes #4549
Fixes #4448