Ensure we use PST colours throughout #1927
Conversation
trallard
added
kind: enhancement
| @@ -61,12 +61,6 @@ a > code { | |||
| color: var(--pst-color-inline-code-links); | |||
| } | |||
|
|
|||
| // Fix for Sphinx's "highlight" directive - this is an issue with our accessible pygments theme | |||
There was a problem hiding this comment.
No longer needed now that we fixed accessible pygments
| // calculate the text color for the highlight color | ||
| --pst-color-#{$name}-highlight-text: #{a11y-combination($highlight-color)}; |
There was a problem hiding this comment.
Since we already calculate a WCAG conformant text colour for our semantic colours it made sense to do the same for highlight colours that are used for hover
| // applying the same style as the SD "buttons" | ||
| @include link-style-hover; | ||
|
|
||
| text-decoration-thickness: 1px; |
There was a problem hiding this comment.
Seems this was missed, so I added this to bring parity with other buttons @gabalafou unless there was a reason to leave this out?
There was a problem hiding this comment.
Correct. Plus, I like this parity because the text underline suggests that the button is actually a link, which is the case.
There was a problem hiding this comment.
I wanted to comment that on the issue but I only got to it now and I see there is a PR already, thanks for the quick response @trallard!
I see now that these colors are always available, not only when sphinx-design is an active extension which was confusing me when I wrote the issue.
|
Great to see this being fixed, as I was also trying to change the hover color of the 'back-to-top' button via a custom css file! However I saw that the same goes for the hover color for tables (this is also hard coded to a certain violet value in _color.scss ) Maybe that can also be changed? I am a css rookie so maybe I am mistaken and it isn't hard coded, in that case could you point me a way to edit this? :) |
|
You can already change the table hover, it will be the color of whatever you have defined as |
Ah okay, thats great. I had been staring at the color userguide and trying different things for a few hours, but the "-bg" suffix requirement wasn't clear to me. Thank you for the quick response, the example and the help! |
Not so long ago, we changed our colour system, and therefore how we define colours, so it looks like this could be better documented/updated in the docs. |
src/pydata_sphinx_theme/assets/styles/extensions/_sphinx_design.scss
Outdated
Show resolved
Hide resolved
|
|
||
| // TODO: highlight seems to be used for buttons @trallard to fix on a11y follow-up work | ||
| // highlight is used for hover effects on buttons, here we make some fluid scaling | ||
| // to avoid jarring effects |
There was a problem hiding this comment.
Could we delete lines 91-93? The same comment is made earlier, I think repeating it here adds noise
There was a problem hiding this comment.
Yes we can remove them I just substituted the text that was there before.
| @each $name in $sd-semantic-color-names { | ||
| .sd-btn-#{$name} { | ||
| &:hover { | ||
| color: var(--pst-color-#{$name}-highlight-text) !important; |
There was a problem hiding this comment.
Do you think we should try upstreaming things like this?
| @@ -66,25 +66,40 @@ $all-colors: map.merge($pst-semantic-colors, $extra-semantic-colors); | |||
|
|
|||
| @mixin create-sd-colors($value, $name) { | |||
| // define the pst variables, so that downstream user overrides will work | |||
| @debug "Creating color variables for semantic color: #{$name}"; | |||
There was a problem hiding this comment.
It looks like we're able to create AA conforming colors for everything except warning-highlight in light mode:
| warning-highlight | warning-highlight-text |
|---|---|
#d27a07 |
#14181e |
Contrast ratio 4.4:1.
There was a problem hiding this comment.
Yeah there is no WCAG conformant combination with our text colours and the orange (at least how this is being calculated). And I did not want to spend hours and hours tweaking that to get perfect colours considering we only use these for hover effects.
The alternative would be to manually specify all the colours pst and sd and use our colour palette.
I mean we can do that since the colours are always present like mentioned in the comment above.
But there are tradeoffs to each approach.
There was a problem hiding this comment.
That makes sense. I was actually trying to say that I was impressed that there was only one semantic color that didn't work. And it's not that far from the 4.5 target.
…n.scss Co-authored-by: gabalafou <gabriel@fouasnon.com>
Coverage report
This PR does not seem to contain any modification to coverable code. |
Closes #1925
Also, as I was fixing the above issue, I took the opportunity to make some needed changes regarding text colour.
And I realised there was perhaps a missing style from the banner button.