-
Notifications
You must be signed in to change notification settings - Fork 856
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
Pastel & Hot Pink Themes #3937
Pastel & Hot Pink Themes #3937
Conversation
…or instead of --scrollbar-color This unneeded dependency on the scrollbar color here resulted in the Author Background text blending into its background on the new Hot Pink theme. This change does not tangibly affect any other theme, whose scrollbar colors are incidentally similar to the secondary-card-bg-color, as this color is already being used for tags on the Channel About tab.
Note that Hot Pink styling overwrites the primary and secondary color themes to maintain accessible color contrasts throughout FreeTube. It also updates the underline styling to match its aesthetic. The scrollbar-text-color-hover and side-nav-active-text-color colors are added for allow for the text and icon to both change on hover. This allows for more accessible hover and active styling.
The textWhiteSmall was actually already sized exactly the same as the other small text files, so I just changed the name to reflect that fact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything LGTM except for one minor thing
If u switch to the hot pink theme and open another settings tab u will see that the color of the toggles changes from blue to black. This is also the case if u switch back to a theme that uses the secondary color for the toggles, u will see the switch from black to blue
VirtualBoxVM_yzFkBmB3VU.mp4
Hi @efb4f5ff-1298-471a-8973-3d47447115dc, does this section answer your question?
Edit: I think I will visibly disable the primary and secondary theme colors when hot pink is selected to make this clearer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Pastel & Hot Pink Themes
Pull Request Type
Related issue
closes #3925
Description
This PR adds one pastel pink theme and one hot pink theme.
Notes:
commentOwner
styling has been updated to use the more fitting--secondary-card-bg-color
color (a la on the tags on the Channel: About tab) instead of--scrollbar-color
. This unnecessary coupling led to a minor issue with one of the new themes, and its presence would be unnecessarily complicated for future themes going forward. This has minimal effect on existing styles.--scrollbar-text-color-hover
,--side-nav-hover-text-color
, and--side-nav-active-text-color
colors have been implemented throughout FT to allow for modifying the text color styling as well when--scrollbar-color-hover
,--side-nav-hover-color
, and--side-nav-active-color
are used. This allows for a greater degree of control over hover and active styling on controls possessing two colors. That is especially useful for creating acceptable color contrasts on:hover
and:active
(recommended 4.5:1) compared to the base control. I used it for this purpose in the Hot Pink theme, and I imagine this will be quite useful for new themes going forward. (As an aside, "scrollbar text color" sounds somewhat silly, but as long as we keep using--scrollbar-color
on things that aren't scrollbars, it's just the cost of doing business.)Screenshots
Pastel Pink
Hot Pink
Testing
Every color in both of these palettes has been carefully and painstakingly selected to ensure both satisfaction of WCAG 2 Level AA and a pleasant user experience. Close attention was paid to
:hover
and:active
states for all controls.Pastel Pink link styling color contrast seen here.
Hot Pink link styling color contrast seen here.
Desktop
Additional context
I'm using dashes (i.e.,
pastel-pink
,hot-pink
) to represent the color names inindex.js
in accordance with a previous merged inclusion, but I'm not fully certain how that works out given that they're originally written in camelCase.