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.

Sign up for GitHub

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

Ensure we use PST colours throughout #1927

Merged
merged 11 commits into from
Aug 6, 2024
Merged

Ensure we use PST colours throughout #1927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
848fc2f
ENH - Remove hover hard coded colour
trallard Jul 12, 2024
43e4df8
Add primary and secondary button examples to docs
trallard Jul 12, 2024
f58dbee
Tweak colour scaling slightly
trallard Jul 12, 2024
959bb4f
Remove hack fix - no longer needed
trallard Jul 12, 2024
2b0d8d3
Add calculation for highlight text colour
trallard Jul 15, 2024
57a2384
Set hover text colour to accessible colour
trallard Jul 15, 2024
29875a8
Rename colours to pst
trallard Jul 15, 2024
d210f95
Add missing style from banner button
trallard Jul 15, 2024
5c66a78
Fix broken link in docs
trallard Jul 15, 2024
3a8609a
Update src/pydata_sphinx_theme/assets/styles/extensions/_sphinx_desig…
trallard Jul 29, 2024
dcbade5
remove comment
gabalafou Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions docs/user_guide/web-components.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ Sphinx Design Components
On this page, you will find user interface components such as badges, buttons,
cards, and tabs.

The components on this page are **not provided by PyData Theme**. They are
provided by `Sphinx Design <https://sphinx-design.readthedocs.io/en/pydata-theme/index.html>_` (a Sphinx extension). This means that if you wish
to use the components on this page, you must install Sphinx Design separately and add it to your `conf.py`.
The components on this page are **not provided by PyData Theme**.
They are provided by `Sphinx Design <https://sphinx-design.readthedocs.io/en/pydata-theme/index.html>`_ (a Sphinx extension).
This means that if you wish to use the components on this page, you must install Sphinx Design separately and add it to your `conf.py`.

.. seealso::

Expand Down Expand Up @@ -111,6 +111,24 @@ Here are some of the available button-style links, also using semantic colors:

Dark

.. grid-item::

.. button-ref:: badges-buttons
:ref-type: ref
:color: secondary
:shadow:

Secondary

.. grid-item::

.. button-ref:: badges-buttons
:ref-type: ref
:color: primary
:shadow:

Primary

gabalafou marked this conversation as resolved.
Show resolved Hide resolved
.. note::

`Sphinx Design buttons
Expand Down
4 changes: 2 additions & 2 deletions src/pydata_sphinx_theme/assets/styles/base/_base.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,8 @@ pre {
@include link-style-hover;

text-decoration-thickness: 1px;
background-color: var(--pst-violet-600);
color: var(--pst-color-secondary-text);
background-color: var(--pst-color-secondary-highlight);
color: var(--pst-color-secondary-highlight-text);
}

&:focus-visible {
Expand Down
6 changes: 0 additions & 6 deletions src/pydata_sphinx_theme/assets/styles/content/_code.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No longer needed now that we fixed accessible pygments

// and the colour we are using for the background of the code blocks.
html[data-theme="light"] .highlight .nf {
color: #0078a1 !important;
}

// Minimum opacity needed for linenos to be WCAG AA conformant
span.linenos {
opacity: 0.8 !important;
Expand Down
26 changes: 21 additions & 5 deletions src/pydata_sphinx_theme/assets/styles/extensions/_sphinx_design.scss
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -66,25 +66,33 @@ $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}";
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@trallard trallard Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


--pst-color-#{$name}: #{$value};

// we are now using a11y-combination to calculate the text color - this is based
// on the WCAG color contrast guidelines
--pst-color-#{$name}-text: #{a11y-combination($value)};

// TODO: highlight seems to be used for buttons @trallard to fix on a11y follow-up work
--pst-color-#{$name}-highlight: #{color.adjust($value, $lightness: -15%)};
// highlight is used for hover effects on buttons, here we make some fluid scaling
// to avoid jarring effects -- we create a local variable that we can access
// later to calculate the text color
$highlight-color: color.scale($value, $lightness: -15%, $saturation: 5%);

--pst-color-#{$name}-highlight: #{$highlight-color};

// override the sphinx-design variables
--sd-color-#{$name}: var(--pst-color-#{$name});
--sd-color-#{$name}-text: var(--pst-color-#{$name}-text);

// TODO: highlight seems to be used for buttons @trallard to fix on a11y follow-up work
--sd-color-#{$name}-highlight: var(--pst-color-#{$name}-highlight);

// calculate the text color for the highlight color
--pst-color-#{$name}-highlight-text: #{a11y-combination($highlight-color)};
Comment on lines +89 to +90
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

}

// Now we override the --sd-color-* variables.
@each $mode in (light, dark) {
@debug "Creating color variables for mode: #{$mode}";
html[data-theme="#{$mode}"] {
// check if this color is defined differently for light/dark
@each $name in $sd-semantic-color-names {
Expand All @@ -99,7 +107,7 @@ $all-colors: map.merge($pst-semantic-colors, $extra-semantic-colors);
@if string.index($key, "bg") {
--sd-color-#{$name}-bg: #{$value};

// create local variable
// create local variable -- needed to calculate the text color
$value: check-color($value);

--sd-color-#{$name}-bg-text: #{a11y-combination($value)};
Expand Down Expand Up @@ -341,6 +349,14 @@ html {
}
}

@each $name in $sd-semantic-color-names {
.sd-btn-#{$name} {
&:hover {
color: var(--pst-color-#{$name}-highlight-text) !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think we should try upstreaming things like this?

}
}
}

@each $name in $sd-semantic-color-names {
.sd-btn-#{$name},
.sd-btn-outline-#{$name} {
Expand Down
6 changes: 5 additions & 1 deletion src/pydata_sphinx_theme/assets/styles/sections/_announcement.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,13 @@
border-radius: 0.25rem;

&:hover {
// applying the same style as the SD "buttons"
@include link-style-hover;

text-decoration-thickness: 1px;
Comment on lines +56 to +59
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems this was missed, so I added this to bring parity with other buttons @gabalafou unless there was a reason to leave this out?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct. Plus, I like this parity because the text underline suggests that the button is actually a link, which is the case.

background-color: $hover-background-color;
border-color: $hover-background-color;
color: $color;
color: var(--pst-color-danger-highlight-text);
}

&:focus-visible {
Expand Down
Loading