WordPress / openverse

Openverse is a search engine for openly-licensed media. This monorepo includes all application code.
https://openverse.org
MIT License
214 stars 176 forks source link

Semantics for the new color system #4325

Open fcoveram opened 1 month ago

fcoveram commented 1 month ago

Part of #3592

Description

As part of the new color system introduced with the dark mode designs #3959, the color scales asked to be implemented in #4268 have a semantic naming to describe how they are applied in Openverse UI.

This ticket details the alias for light and dark themes as CSS variables.

Light theme

:root {
    bg: var(white);
    bg-surface: var(gray-1);
    bg-overlay: var(white);
    bg-fill-primary: var(pink-8);
    bg-fill-primary-hover: var(pink-9);
    bg-fill-secondary: var(gray-2);
    bg-fill-secondary-hover: var(gray-12);
    bg-fill-terciary: var(gray-12);
    bg-fill-terciary-hover: var(gray-11);
    bg-fill-transparent-hover: var(gray-12-10);
    bg-fill-complementary: var(yellow-3);
    bg-fill-warning: var(warning-2);
    bg-fill-info: var(info-2);
    bg-fill-success: var(success-2);
    bg-fill-error: var(error-2);
    bg-fill-disabled: var(gray-5);
    bg-zero: var(white-0);
    border: var(gray-3);
    border-hover: var(gray-12);
    border-secondary: var(gray-12-20);
    border-secondary-hover: var(gray-12);
    border-tertiary: var(gray-12);
    border-transparent-hover: var(gray-3);
    border-focus: var(pink-8);
    border-bg-ring: var(white);
    border-disabled: var(gray-5);
    text: var(gray-12);
    text-secondary: var(gray-8);
    text-disabled: var(gray-5);
    text-link: var(pink-8);
    text-over-dark: var(white);
    text-secondary-over-dark: var(gray-5);
    icon-warning: var(warning-8);
    icon-info: var(info-8);
    icon-success: var(success-8);
    icon-error: var(error-8);
    wave-active: var(yellow-9);
    wave-inactive: var(gray-12-20);
    modal-layer: var(gray-12-80);
}

Dark theme

:root {
    bg: var(gray-13);
    bg-surface: var(gray-12);
    bg-overlay: var(gray-11);
    bg-fill-primary: var(yellow-4);
    bg-fill-primary-hover: var(yellow-3);
    bg-fill-secondary: var(gray-11);
    bg-fill-secondary-hover: var(gray-1);
    bg-fill-terciary: var(gray-1);
    bg-fill-terciary-hover: var(gray-2);
    bg-fill-transparent-hover: var(gray-1-10);
    bg-fill-complementary: var(pink-9);
    bg-fill-warning: var(warning-11);
    bg-fill-info: var(info-11);
    bg-fill-success: var(success-11);
    bg-fill-error: var(error-11);
    bg-fill-disabled: var(gray-8);
    bg-zero: var(gray-13-0);
    border: var(gray-11);
    border-hover: var(gray-1);
    border-secondary: var(gray-1-20);
    border-secondary-hover: var(gray-1);
    border-tertiary: var(gray-1);
    border-transparent-hover: var(gray-11);
    border-focus: var(yellow-4);
    border-bg-ring: var(gray-13);
    border-disabled: var(gray-8);
    text: var(gray-1);
    text-secondary: var(gray-5);
    text-disabled: var(gray-8);
    text-link: var(yellow-4);
    text-over-dark: var(gray-13);
    text-secondary-over-dark: var(gray-8);
    icon-warning: var(warning-5);
    icon-info: var(info-5);
    icon-success: var(success-5);
    icon-error: var(error-5);
    wave-active: var(pink-4);
    wave-inactive: var(gray-1-30);
    modal-layer: var(gray-12-60);
}
zackkrida commented 1 month ago

@fcoveram this is excellent, thank you so much! I thought of something relating to this today. We may need to modify these names for the frontend:

These names have some redundancy with the CSS classes tailwind generates. For example, if we have a color called "primary" tailwind creates classes like "bg-primary", "text-primary", "border-primary", and so on. So if we used these names exactly as-written we would get generated classes like "bg-bg-fill-success" and so on. This isn't necessarily a huge problem but does seem a little inelegant.

Perhaps we could remove the bg-, and bg-fill- prefixes from some of the colors and keep all of the other names the same. We would end up with classes like text-text and border-border but it isn't terrible. @WordPress/openverse-frontend what do you think about this problem?

sarayourfriend commented 1 month ago

Considering tailwind creates these classes for all colours, regardless of their intention, I think it's better to keep the bg and bg-fill prefixes. bg-bg-fill-success is ugly, but dropping the prefix makes classnames like text-surface, which should never be used. If we keep the prefixes, on the other hand, it would be text-bg-surface, which at least gives some indication that it's incorrect usage.

We'd still want to be careful here though because even the possibility of text-bg-surface is important to prevent if we can, through means other than relying on PR reviews catching it. Is it possibly to bypass tailwind's class generation altogether by disabling all tailwind-built colours (looks like colors: {} would do the trick) and then define these classes ourselves? We can probably pretty easily write a script to generate the list programmatically to reduce the burden of making changes in the future :thinking:

Generally I think it would be a good idea to discourage misuse of these variables as much as possible.

fcoveram commented 1 month ago

I agree with @sarayourfriend about the importance of adding information to prevent misuse.

Regarding naming, the difference between bg and bg-fill is that the latter works for individual elements of the UI, while the former is a section on the page. Because of this, there are more bg-fill elements in the collection.

The fill version could be renamed as fill without problem. In that vein, it seems better that Figma variables follow what's best in frontend. The Figma design library doesn't risk breaking the files attached to it.