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: CSS margin overhaul #7657

Merged
merged 7 commits into from
Jun 7, 2020
Merged

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented May 12, 2020

Fixes #7544.

Some details might still be missing, but I think this is at least a step in the right direction.

As mentioned in #7544 (comment):

I think the "right" solution would be to remove the margin-bottom from the :last-child and add a consistent padding: 7px on all sides.
The problem is that this would be quite a big change in multiple themes, and people might consider it a "breaking change".

It turned out that I had only to change the basic theme, but it's still quite a big change, and some might consider it "breaking".

Any suggestions for improvements?

@tk0miya
Copy link
Member

tk0miya commented Jun 7, 2020

I'm not an expert in HTML/CSS. So I don't have a comment on this. So let's see what effect to 3rd party themes after merging.

@tk0miya tk0miya merged commit 2ed2085 into sphinx-doc:3.x Jun 7, 2020
tk0miya added a commit that referenced this pull request Jun 7, 2020
@mgeier mgeier deleted the css-margins branch June 7, 2020 20:03
@return42
Copy link

You should create a PR with your suggested improvements.

Are you joking? Can you image how much work I have to repair all my projects using up to date sphinx-doc (3.1) .. Did you ever compiled the sphinx-doc project itself? Did you ever had a look on long contents directives? Do you ever compiled one of the big searx projects (like python docs) ..

Or did you just fumble with theses small docutils examples?

You should test elementary patches and not release code and test it by the users, awaiting that they send you patches repairing all you fumbled broken.

If you are not knowing what to do just git-revert the patches.

@mgeier
Copy link
Contributor Author

mgeier commented Jun 18, 2020

You should create a PR with your suggested improvements.

Are you joking?

Not at all!
Please make a PR (or multiple)!

Can you image how much work I have to repair all my projects using up to date sphinx-doc (3.1) ..

For the short term, it should be sufficient to pin sphinx<3.1 in your dependencies. So yes, very little work.

Then we can try to fix the issues together, which will need some work done by someone.

Once they are fixed, you can un-pin your dependencies and everything will be honky-dory.

Did you ever compiled the sphinx-doc project itself?

Yes, I did.

Ironically, the Sphinx docs don't use that many Sphinx features.

Did you ever had a look on long contents directives?

Yes, I did.

Do you ever compiled one of the big searx projects (like python docs) ..

No, I've never heard about it.

Or did you just fumble with theses small docutils examples?

Of course not.

I just used the docutils example in #7838 (comment) to illustrate the visual inconsistencies.

A longer list wouldn't add any useful information.

You should test elementary patches and not release code and test it by the users, awaiting that they send you patches repairing all you fumbled broken.

Sure, I tested it quite thoroughly, but I can never guarantee that nothing will break.

Also, it is quite hard to get the attention of potential reviewers for every single PR for Sphinx. But it is really easy to get user's attention after things are released (as your comments have shown).

If you are not knowing what to do just git-revert the patches.

I would say that I know to some degree what I'm doing, but I'm certainly not an expert. I'm very much open to suggestions for improvements!

I think the changes #7657 are a net improvement, so I wouldn't completely revert it.
But there are for sure some things that can still be improved by further PRs.


While I'm at it, please let me also respond to your comment #7838 (comment), in order not to disturb the constructive discussions over there:

@mgeier please stop changing basic CSS !!!

I don't have concrete plans to change further CSS in the basic theme, but I can't guarantee that I won't find further problems that I would like to fix.
But for now, I've already done all the changes I wanted to be done:

i'm pretty annoyed because i have been trying to find out how to fix this broken layout in customizing for hours ... and i can do that in all my projects .. thanks for all the work ..

As mentioned above, ad-hoc work-arounds are probably not the best use of your time.

how do you get the idea to change the basic CSS so massively without having to test it broadly.

Good question!

I just saw that there are problems with the basic CSS and tried to fix them.
I was fully aware that people might complain, but I was also aware that whatever they complain about will also be fixable. And in the end, we all will have a better Sphinx.


... and #7838 (comment):

@mgeier wrotes ..

I would like to argue that the new spacing is more consistent,

Not with the toc directive which is rendered much more compact.

If everything is compact, it's still consistent.
I'm not arguing that compact or non-compact is better, as long as it is consistent.

And I think the discussions in #7838 look very promising. I think it is realistic that we will get compact lists (for you) that are still consistent (for me).

@mgeier
Copy link
Contributor Author

mgeier commented Jun 20, 2020

@return42 Please check out #7852, which should fix the list spacing issues.

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.

classic theme: inconsistent padding in admonitions vs. topic/sidebar
3 participants