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

basic theme: Avoid clashes between sidebar and other blocks #7484

Merged
merged 3 commits into from
Apr 16, 2020

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Apr 14, 2020

When following a sidebar, these blocks should not be squished next to the sidebar, but they should start after the end of the sidebar in their full width:

  • admonitions
  • topics
  • code blocks
  • further sidebars

@mgeier
Copy link
Contributor Author

mgeier commented Apr 14, 2020

I think it might be better to clear only "right" in the case of a sidebar.
Feel free to squash ae9ae3e.

@tk0miya
Copy link
Member

tk0miya commented Apr 15, 2020

Could you paste screenshots for before and after this change?
I can't imagine what is happened now.

@tk0miya tk0miya added this to the 3.1.0 milestone Apr 15, 2020
@mgeier
Copy link
Contributor Author

mgeier commented Apr 15, 2020

Sure.

.. sidebar:: Sidebar 1

   Text.

.. sidebar:: Sidebar 2

   Text.

.. topic:: Topic

   Text.

Before:

image

After:

image

The second one is better, isn't it?

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

sphinx/themes/basic/static/basic.css_t Outdated Show resolved Hide resolved
@tk0miya
Copy link
Member

tk0miya commented Apr 16, 2020

Thank you for explanation. I've forgotten how sidebars are rendered. +1 for bugfix.

@tk0miya tk0miya merged commit bb6d1d5 into sphinx-doc:3.x Apr 16, 2020
@tk0miya
Copy link
Member

tk0miya commented Apr 16, 2020

Thanks!

tk0miya added a commit that referenced this pull request Apr 16, 2020
@mgeier mgeier deleted the clear-sidebar branch April 16, 2020 18:19
tk0miya pushed a commit that referenced this pull request Apr 23, 2020
@return42
Copy link

The second one is better, isn't it?

depends on the use case .. TLDR; its just another broken solution.

Since you will normally not really often have one after the other sidebar I prefer as it was .. Did you ever seen a contents and a sidebar directive on the top of a document .. example .. "second" looks horrible while first is overlapping, which looks much nicer to me.

before #7484

Bildschirmfoto vom 2020-06-18 16-22-13

after #7484

Bildschirmfoto vom 2020-06-18 16-23-35

@return42
Copy link

FWIW: here is my hot-fix (not widely tested):

div.sidebar {
    clear: none;
}

div.admonition, div.topic, pre {
    clear: none;
}

@mgeier
Copy link
Contributor Author

mgeier commented Jun 18, 2020

I'm quite sure that the changes for div.sidebar and for code blocks are good for all themes.
@return42 Would you agree?

I admit that I might have been over-eager in adding div.admonition and div.topic into the mix.
It would probably be better to remove those from the basic theme and add them individually to those themes where it is necessary.

@return42 Would you like to make a PR for this (I think it would be good to use the 3.1.x branch)?

@mgeier
Copy link
Contributor Author

mgeier commented Jun 27, 2020

I'm suggesting to partially revert this PR in #7878.

mikeri pushed a commit to mikeri/searx that referenced this pull request Mar 7, 2021
This reverts commit 0616684.

Since PR sphinx-doc/sphinx#7878 has been merged into
Spinx-doc (v3.1.2), this patch is no longer needed:

  See sphinx-doc project, PR 7838 & 7484 with elementary patch to the basic CSS:

  - sphinx-doc/sphinx#7838 (comment)
  - sphinx-doc/sphinx#7484 (comment)

Signed-off-by: Markus Heiser <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants