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

ENH: Improve quotes display CSS #743

Merged
merged 10 commits into from
Jun 22, 2022
Merged

ENH: Improve quotes display CSS #743

merged 10 commits into from
Jun 22, 2022

Conversation

12rambau
Copy link
Collaborator

Fix #454

The quotes were using a legacy color.
Used the now-classic trick of admonition to display quotes in light grey using a left border (grey as well. It looks more like what we see on other platforms like GitHub and is working with any quote directive including epigraph, highlight and pull-quote.
Capture d’écran 2022-06-20 à 10 21 20

@12rambau 12rambau marked this pull request as ready for review June 20, 2022 08:38
Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks good in general - two quick thoughts:

  • The text doesn't seem to be selectable, I wonder if the &after is floating above it in the blockquotes?
  • Could we add some padding just above and below the text? I think we could just re-use the same top/bottom padding that github uses

src/pydata_sphinx_theme/assets/styles/content/_quotes.scss Outdated Show resolved Hide resolved
@choldgraf
Copy link
Collaborator

Looking better! Three quick thoughts:

  1. The padding in literal blocks is twice as much as others.

    chrome_R2kEFtDmOJ
  2. There seems to be more padding on the bottom than the top. I think we should remove the bottom margin only for the last element in the quote. For example:

    chrome_NVYMyPTPVJ
  3. Also just a note I still cannot select text. When I hover over the quote block, my cursor does not register as hovering over text. I think something exists above the text in the quote blocks

@12rambau
Copy link
Collaborator Author

The padding in literal blocks is twice as much as others

I removed to margins of the line-block when they are in <blockquote>.

There seems to be more padding on the bottom than the top

There is a margin-bottom associated with <p> element in the theme. It's now removed for the last child

Also just a note I still cannot select text.

There is nothing on top of the <blockquote>. They are behaving the same way as the title of admonitions and deprecated blocks i.e. the cursor stay unchanged but the text can be copied as usual (at least on my computer using the latest Safari browser). I assume it's related to the ::before pseudo element that forces the cursor.

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

Just one quick comment in there, if that resolves it then I think we should merge

@choldgraf
Copy link
Collaborator

choldgraf commented Jun 22, 2022

OK so setting z-index: -1 does fix the text selection problem, but it results in the admonitions no-longer having colored title areas. That is because the admonitions also set a background color, and this is then "higher" than the title area and so covers that color entirely.

I think there are 3 ways we could get around this:

  1. Define the z-index: -1 as a special-case hack just for the quote blocks (or other blocks where we want the text selectable).
  2. Remove background-color from the admonitions and just have them be transparent
  3. Find a way to make the z-index of the admonition background still be behind the admonition title

I think it's going to confuse people that they can't obviously select text inside of quotes, so IMO it's something we should try to fix (I know it's technically possible via double clicking and such but that feels like a weird UX to me)

@12rambau
Copy link
Collaborator Author

We initially set back the admonition background when we were playing with the colors to increase the contrast in the dark background so I would say we really want to keep it. I will go with the simple z-index hack for this specific use case. it's harmless and works just fine.

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks good to me! Let's give it a go! Thanks @12rambau

@choldgraf choldgraf changed the title improve quotes display ENH: Improve quotes display CSS Jun 22, 2022
@choldgraf choldgraf merged commit 7b5f453 into pydata:main Jun 22, 2022
@12rambau 12rambau deleted the directive branch June 22, 2022 09:49
@jarrodmillman jarrodmillman added this to the 0.10 milestone Jul 26, 2022
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.

Support some common directives
3 participants