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

Add spacing between buttons and code #1508

Conversation

MatheusRich
Copy link

Problem

Sometimes the buttons in code listings are above the code itself, making it hard to read. Here's some examples:

image
image

Solution

I've added a vertical padding to help separate the code from the buttons:

image
image

I could've added just a padding-top, but I thought it was ugly:

image

If that's not the right file for css changes, let me know!

@ehuss
Copy link
Contributor

ehuss commented Apr 25, 2021

Hi, thanks for the PR! There is a bit of uncertainty about exactly what the behavior should be here. There is more discussion in #1374 (comment) where there is a PR that does the opposite where it collapses the spacing. Issues #1322 and #1433 also contain some more discussion.

One issue with this PR is that it adds too much spacing for rust code blocks that have an injected main.

image

I'm wondering about maybe making the spacing depend on the screen width? Like on a narrow screen, it would have the increased spacing unconditionally? That could still have some overlap issues if the first line is very long, but might be something to try.

@MatheusRich
Copy link
Author

MatheusRich commented Apr 25, 2021

We could add a media query so the spacing would be applied only on small screens. What do you think? It's possible to move the buttons outside the code block too.

@ehuss
Copy link
Contributor

ehuss commented May 4, 2021

A media query seems worth trying. I'm not sure how to detect the "injected main" case, or if that should be changed (I don't remember why they are different).

@MatheusRich
Copy link
Author

@ehuss I've added padding-top with media queries. See if that fits what you're thinking of.

@serverwentdown
Copy link

This change is pretty good with media queries!

Copy link

@serverwentdown serverwentdown left a comment

Choose a reason for hiding this comment

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

I think theme is the wrong place to put the padding tweaks. Maybe place the change here instead.

@serverwentdown
Copy link

serverwentdown commented Sep 30, 2021

Now the padding inserted is outside the box, maybe you can try this:

pre > .buttons ~ .hljs {
padding-top: 2em;
}

@MatheusRich MatheusRich force-pushed the add-spacing-between-buttons-and-code branch from 19e28aa to f087866 Compare September 30, 2021 02:06
@MatheusRich
Copy link
Author

@serverwentdown yeah, that seems to work!

@serverwentdown
Copy link

My bad, .hljs would break if the syntax highlighter changes, so we shouldn't depend on that class. code should be better:

pre > .buttons ~ code {
padding-top: 2em;
}

(The above reads as "Any pre element containing a direct child .button that also contains a subsequent code element. Pick that code element.")

@MatheusRich MatheusRich force-pushed the add-spacing-between-buttons-and-code branch from f087866 to e214830 Compare September 30, 2021 02:24
@MatheusRich MatheusRich force-pushed the add-spacing-between-buttons-and-code branch from e214830 to e58cf54 Compare September 30, 2021 13:14
@MatheusRich
Copy link
Author

Is there anything we can do to move this forward?

@ehuss
Copy link
Contributor

ehuss commented May 17, 2022

I think I would like to go in a slightly different direction here. I posted #1806 with a proposed fix.

I would appreciate if you could take a look and see if you have any thoughts. I posted a demo at https://ehuss.github.io/rust-book/. If you could look through some examples, try different browsers and platforms, different color themes, etc. If you have access to Android, that would particularly be nice to test since I don't have access to that.

@ehuss
Copy link
Contributor

ehuss commented May 28, 2023

I'm going to close as I think this should mostly be resolved by #1806. If there are further ideas to improve the layout, feel free to open an issue with some screenshots. I appreciated the help and discussion here, hopefully the primary concern is resolved.

@ehuss ehuss closed this May 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants