-
-
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
Version warning banner: inject on role="main" or main tag #8079
Conversation
9e03f59
to
0c77197
Compare
body = $("div.document"); | ||
var selectors = ['[role=main]', 'main', 'div.body', 'div.document']; | ||
for (var i = 0; i < selectors.length; i += 1) { | ||
var body = $(selectors[i]); |
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.
Were we ok using let
instead of var
? Or there was some browser compatibility we were worried about?
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.
FWIW, if we're using let, we should also use it (and const) as much as possible.
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.
We should keep it the same as the existing code, to keep things consistent. Then we can change it up when we decide to at the project level.
@@ -145,7 +145,23 @@ you can enable it in the admin section of your docs (:guilabel:`Admin` > :guilab | |||
|
|||
.. note:: | |||
|
|||
This feature is available only for :doc:`Sphinx projects </intro/getting-started-with-sphinx>`. |
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.
Is this not true anymore?
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 this, but I was wrong, thought we injected this from our extension, but this is just JS from the footer. Well, it wasn't a complete lie, since isn't common in mkdocs themes to have div.body or div.document p:
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 seems like a useful change. I do wonder if it will break something (changes always break something..), but hopefully it works better in most cases.
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 know this tick isn't gonna be green, but I do want to note that I reviewed this and I like it. :)
var selectors = ['[role=main]', 'main', 'div.body', 'div.document']; | ||
for (var i = 0; i < selectors.length; i += 1) { |
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 think we should copy these lines to the sphinx-version-warning
extension. Currently, it only uses one selector and the user can change it via a config, but I suppose we could try these same different selectors by default: https://github.com/humitos/sphinx-version-warning/blob/master/versionwarning/_static/js/versionwarning.src.js#L21
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.
Ref pradyunsg/sphinx-basic-ng#12