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

STYLE: style admonitions #1043

Merged
merged 7 commits into from
Nov 8, 2022
Merged

STYLE: style admonitions #1043

merged 7 commits into from
Nov 8, 2022

Conversation

12rambau
Copy link
Collaborator

@12rambau 12rambau commented Oct 27, 2022

related to #1040

The idea here is to align the styling of our admionitions on what's used in other themes so that user don't get lost when switching to ours. In short the RTD theme is driving everything as it's the one by default when you publish on their site so I think we can consider their coloring as default. some themes are stricly respecting it (picolo, insipid, alabaster, material). Furo stands out as every admonition have a color variation on RTD theme (important green is not the same as hint green).

Thus what I changed in this PR:

  • use text justify for custom admonitions (inspired by Furo)
  • use grey color and pencil icon for todo admonitions
  • fallback to blue for seealso

everything can be checked on this page:

@12rambau 12rambau marked this pull request as ready for review October 27, 2022 09:21
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 seems good to me - curious what @stevepiercy thinks

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

this LGTM except for one comment below about the icon for "generic" admonitions. As mentioned in #1040 I'm also ok with @stevepiercy's idea to change color of "attention" admonitions to match color of "important" admonitions (they already have the same icon). But I'm also fine to leave as-is (for consistency with RTD theme).

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

I like the results (this page looks good to me) but I'm a bit perplexed by the implementation. Specifically, the system of "primary/secondary -> structural elements" and "semantic colors -> admonitions" is mixed up now:

  • --pst-color-link-hover is now defined as the warning color instead of secondary
  • styles for attention and important refer to --pst-color-secondary instead of a semantic color

I was expecting a newly added variable like --pst-color-attention or --pst-color-important to be the yellow color, but instead secondary color has changed from orange to yellow. I realize that the end result is the same for our docs pages but downstream users of the theme might be re-using our variables like --pst-color-secondary for other parts of their site, and the PR as currently implemented will cause an unexpected orange -> yellow change for them.

@12rambau
Copy link
Collaborator Author

12rambau commented Nov 1, 2022

@drammock I decided to change the secondary color for 2 reasons:

  • it was only used by the links in our documentation (so I think users don't even know it exists)
  • it was the same color as warningmaking it redundant.

so by changing --pst-color-secondary I introduce a diff between 2 semantic colors (warning and secondary) and the previously used color is still there (as --pst-color-warning) without adding an extra variable.

I can change it back but that seamed reasonable when I did it

@drammock
Copy link
Collaborator

drammock commented Nov 1, 2022

I think users don't even know it exists

I disagree. We use --pst-color-secondary in 5 places in our custom CSS for MNE-Python, so that various elements have similar color behavior to link hovering. And the color variables are advertised in our theme's docs, which say:

There are two special color variables for primary and secondary theme colors (--pst-color-primary and --pst-color-secondary, respectively).

To your second point:

it was the same color as warning making it redundant

That is, IMO, not a problem. The same is true of primary and info. I think it's weird if all the admonitions have an associated semantic color name variable except for one or two of them. Having a complete suite of semantic color name CSS variables makes maintenance easier, and importantly gives users the option to alter the secondary color without affecting admonitions. If someone wants their link hovers to be purple, we shouldn't force them to do extra work to prevent their attention admonitions from also changing to purple.

@choldgraf
Copy link
Collaborator

The demo page looks good to me as well!

I think if @drammock notes that mne uses the secondary color in many other places, we should assume other projects do as well. I'm +1 on merge once his suggestions are addressed!

@scmmmh
Copy link
Contributor

scmmmh commented Nov 7, 2022

Would it be possible to also check that the updated new colours have sufficient contrast for accessibility purposes? The --pst-color-primary has insufficient contrast, which creates a lot of noise in the accessibility contrast reports, but it would be good to check that the new colours have sufficient contrast to be accessible in both the light and dark themes. Then they won't have to be fixed in the near future to improve the accessibility.

@drammock
Copy link
Collaborator

drammock commented Nov 7, 2022

Would it be possible to also check that the updated new colours have sufficient contrast for accessibility purposes?

+1 although I'd prefer that go in it's own PR so we don't delay this one any further.

@choldgraf
Copy link
Collaborator

Agree w/ @drammock - since this PR isn't about the default colors already, let's open an issue to discuss and track the need for accessible default colors, and fix that in a follow-up PR.

@scmmmh
Copy link
Contributor

scmmmh commented Nov 8, 2022

Fine by me. It is the eternal pull between clean, self-contained changes and fixing all the issues. I'll start making a list of colours with contrast issues in a separate issue.

@stevepiercy
Copy link

I'm OK with these changes.

I dislike orange, and think it should be replace by yellow. All of the orange and yellow admonitions are "close enough" semantically not to merit distinct colors. One fewer color to wrangle in regards to accessibility is a win, too.

@choldgraf
Copy link
Collaborator

choldgraf commented Nov 8, 2022

@scmmmh the main reason that the colors conversation might be trickier is because the two main default colors of the theme are defined by the PyData brand, and not unique to this theme. So the question of "should we change the default primary and secondary color?" will also require answering "should this theme's default style break from the PyData brand colorscheme?" and that's more complex than just deciding on the right a11y-friendly color value.

re: Orange vs. yellow, I think the reason we see orange in this theme is for the same reason - it is one of the two PyData brand colors. However, regarding this PR and just for the sake of admonitions, I would be fine making the orange to instead be yellow (although I suspect that yellow will have worse a11y-friendly contrast than orange). If we can all align on this quickly then let's do it, but if it requires conversation lets do that in a follow-up issue.

@scmmmh
Copy link
Contributor

scmmmh commented Nov 8, 2022

@choldgraf Yes, I see the problem. In part that is why I added my comment. When making the accessibility fixes is disconnected from the initial design step, then changing can be trickier, as there are often other dependencies that have arisen since the design decision. The change is probably even trickier, because the contrast is insufficient in both the light and dark themes, but to make it compliant would need to be changed differently for each of the two themes.

@drammock
Copy link
Collaborator

drammock commented Nov 8, 2022

OK I've pushed a commit to address these comments, and the doc builds page still looks good to me: https://pydata-sphinx-theme--1043.org.readthedocs.build/en/1043/examples/kitchen-sink/admonitions.html#admonitions

@choldgraf or @12rambau please merge unless you notice a mistake in my last commit.

@12rambau
Copy link
Collaborator Author

12rambau commented Nov 8, 2022

sorry for not pushing it. I thought it was already done. thanks @drammock

@12rambau 12rambau merged commit 92045be into pydata:main Nov 8, 2022
@12rambau 12rambau deleted the admonitions branch November 8, 2022 20:17
@choldgraf
Copy link
Collaborator

Thanks so much everybody for the discussion and thanks @12rambau for getting it through!

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.

5 participants