-
-
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
Add an explicit flyout placement option #9357
Conversation
This PR does a few things: * Lets themes define a `#readthedocs-embed-placement` element, where we will inject the footer * Documents the flyout menu under Advanced Features Refs pydata/pydata-sphinx-theme#705
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.
Great docs addition 👍 Something we've needed for a very long time
docs/user/flyout-menu.rst
Outdated
HTML themes can style the flyout to make it match the overall style of the HTML. | ||
By default the flyout has it's `own CSS file <https://github.com/readthedocs/sphinx_rtd_theme/blob/master/src/sass/_theme_badge.sass>`_, | ||
which you can look at to see the basic CSS class names. | ||
It also uses fontawesome to define some basic UI elements, |
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.
It also uses fontawesome to define some basic UI elements, | |
It also uses Font Awesome to define some basic UI elements, |
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.
There is a PR opened to remove this and hardcode only the icons used. So, this may conflict with that. See #9297
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'll go ahead and remove this mention for now, so we don't have to remember to remove it. That does seem like a better approach.
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.
Note: Font Awesome isn't being removed, just the need for font files. It's still a Font Awesome licensed graphic.
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.
Looks 💯
docs/user/flyout-menu.rst
Outdated
HTML themes can style the flyout to make it match the overall style of the HTML. | ||
By default the flyout has it's `own CSS file <https://github.com/readthedocs/sphinx_rtd_theme/blob/master/src/sass/_theme_badge.sass>`_, | ||
which you can look at to see the basic CSS class names. | ||
It also uses fontawesome to define some basic UI elements, |
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.
There is a PR opened to remove this and hardcode only the icons used. So, this may conflict with that. See #9297
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.
LGTM!
This isn't a blocker, but a future feature request -- would it be possible to make the CSS styling of the flyout not get injected, if a certain attribute is set on the object? This could look like adding an additional class on the element injected in that case, and scoping the styles to a |
@pradyunsg Yea, I just had a conversation with @agjohnson, and he suggested using |
That sounds great! :) |
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.
Looks good to me! The code is simple 👍🏼 I haven't QAed this locally, tho 😬
What can I do to help this get landed and deployed, so that the various active downstream themes can switch to this? :) |
FYI: The PR description needs to be updated to reflect the implementation (which uses |
@pradyunsg Going to merge this and deploy it tomorrow 👍 |
@pradyunsg This should now be live, but I haven't heavily tested it other than ensuring it injects properly. Please let me know if it's working for you! |
@ericholscher I just realized that the example used in the documentation does not have translations. I think it would be good to improve the example by adding translations so theme authors have a full flyout example and can style it properly. What do you think? |
@humitos Sounds good to me 👍 I just copied an existing style, but we should definitely be documenting (and probably have an example) of the full possible content. |
This PR does a few things:
#readthedocs-embed-flyout
element, where we will inject the footerRefs pydata/pydata-sphinx-theme#705