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

Update button and link colors for themes #2026

Merged
merged 9 commits into from
Jan 18, 2025

Conversation

hichemfantar
Copy link
Contributor

@hichemfantar hichemfantar commented Jan 17, 2025

Adjust button title color and link colors to improve visibility in both dark and light themes.

image

@hichemfantar hichemfantar marked this pull request as ready for review January 17, 2025 17:56
Copy link

github-actions bot commented Jan 17, 2025

@josevalim
Copy link
Member

Thank you @hichemfantar. One issue with this change is that we also use ExDoc to generate docs for Erlang, and now links in Erlang and Elixir would look different. Plus links in Erlang would look in red, which was too distracting. I am afraid we may not go ahead with this one. :(

@hichemfantar
Copy link
Contributor Author

Let me see what it looks like in erlang and I’ll get back to you

@josevalim
Copy link
Member

I believe you can do mix build --proglang erlang or mix docs --proglang elixir. But generally we would want to keep the content between the two consistent.

@hichemfantar
Copy link
Contributor Author

hichemfantar commented Jan 17, 2025

light mode already looks good but i changed to a lighter shade in dark mode and it looks good

current
image

after
image

passes wcag
image

light
image

elixir
image

@josevalim
Copy link
Member

Yes, I think the red links are too jarring, sorry. :( If the current ones do not offer enough contrast, then we can address that, but it would be best if the link colors are not theme dependent. Thanks!

@hichemfantar
Copy link
Contributor Author

here's another sample zoomed out, you can see it's hard to distinguish links from regular text (the red is also easier on the eyes because it feels the bold white text is too bright)

image

image

@hichemfantar
Copy link
Contributor Author

hichemfantar commented Jan 17, 2025

also there needs to sufficient contrast between link text colors and non link text colors. it's why browsers by default use blue for links

image

@josevalim
Copy link
Member

I actually think using blue, the browser default, would be a great idea. It is not theme specific and addresses all issues. WDYT?

@hichemfantar
Copy link
Contributor Author

sounds good, i'll give it a shot

@hichemfantar
Copy link
Contributor Author

hichemfantar commented Jan 17, 2025

should be good now, using the default chrome link colors

@josevalim
Copy link
Member

Thanks. I think there are some regressions:

Screenshot 2025-01-17 at 21 49 05

I believe the Callbacks, Functions, etc should not use the browser style. I'd also change the underline below links to follow browser style as well. Have them in black feel a bit weird, wdyt?

@josevalim
Copy link
Member

The plot thickens. It seems we already have a blue color for links, which is what we use for "code links" in Erlang:

Screenshot 2025-01-17 at 21 52 14

We should probably make it so that both links and code links follow the same style and same color in both Erlang/Elixir. I think this PR still keeps Elixir code links in a purpleish color.

@hichemfantar
Copy link
Contributor Author

hichemfantar commented Jan 17, 2025

i believe we have reached the ending of this story.
everything now should work as expected and underline uses the same color as the link.

@hichemfantar
Copy link
Contributor Author

perhaps get rid the of the noUnderline variables and just name them links?
linksNoUnderline -> links
linksNoUnderlineVisited -> linksVisited

@hichemfantar
Copy link
Contributor Author

renamed the variables to be clearer and got rid of duplicates

@hichemfantar
Copy link
Contributor Author

please tell me everything looks good on your end so we could put this saga to an end

@hichemfantar
Copy link
Contributor Author

Web Content Accessibility criteria here we come

--linksNoUnderline: var(--mainLightest);
--linksNoUnderlineVisited: var(--mainLight);
--link-color: var(--mainLightest);
--link-visited-color: var(--mainLight);
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, to be clear, I was thinking we should move these out of the language custom props and use the same for both languages (so they would go to the general prop definitions). I think having both languages use the same colors for links would be a huge plus for overall ecosystem consistency. Thoughts?

@josevalim
Copy link
Member

I took a look at main and it looks really clean, except the Summary headers and the API reference pages are still being shown as links, as they should have their previous styling (we consider them to be headers):

Screenshot 2025-01-18 at 09 23 31

They already have .summary and .summary-signature headers, so it should be easy to address. Thoughts?

@hichemfantar
Copy link
Contributor Author

I think they should remain clear as links, i actually didn't even know they were clickable until i enabled link coloring.
goes to show you the importance of accessibility guidelines adherence and the ability to be able to differentiate between regular text and clickable links.
I also had an annoying thing happen where i tried to copy the header text and since it wasn't clear that it was a link, i ended accidentally navigating to a different section.

@hichemfantar
Copy link
Contributor Author

btw perhaps get rid of italic because i believe it lowers readability without offering anything?

before
image

after
image

@hichemfantar
Copy link
Contributor Author

hichemfantar commented Jan 18, 2025

@josevalim

I agree with the changes to the footer, including removing the italic, and we can remove the italic in this pull request too

Done

Let's keep the summary headers and signatures in their current color, i.e. black, and discuss their overall style, including italic in another pull request

i agree to move the discussion

Let's make the links color the same in both Erlang and Elixir for overall consistency

i agree with @DavidOliver and think we should keep it

@josevalim
Copy link
Member

It seems I have been overruled. :D Sounds good to me! 💚 💙 💜 💛 ❤️

@josevalim josevalim merged commit c21ef96 into elixir-lang:main Jan 18, 2025
4 checks passed
@hichemfantar hichemfantar deleted the better-links branch January 18, 2025 14:21
@hichemfantar
Copy link
Contributor Author

Here's to better docs 🚀

@DavidOliver
Copy link
Contributor

the current tone and saturation levels pass wcag checks from my testing

Okay, great - thanks. I'll leave the current colour updates, etc., to you and José. Looking forward to trying out all the recent changes soon.

@hichemfantar
Copy link
Contributor Author

feel free to chime in at anytime

@garazdawi
Copy link
Contributor

I was revewing the changes with ex_doc 0.37.0-rc.0 and found that links have changed color in Erlang, but only text links. That is the sidebar and buttons are still red, but the text is blue. Is this intended? For example:

image

image

I don't mind the blue links (though red is Erlang "color", so we should use that when we can), but it feels a bit inconsistent, especially the buttons being red. Thoughts?

@josevalim
Copy link
Member

Originally, ExDoc generated red links for Erlang, but some people found that off-putting, and others complained it was really bad accessibility wise. So it has been a while we have been generating blue links for Erlang. Here is an example for today:

Screenshot 2025-01-27 at 10 00 27

The change here is that we are marking even more things as links, for clarity purposes, but in the sidebar it is an "active" indicator. WDYT?

@garazdawi
Copy link
Contributor

I was mainly making sure that it was an active decision for both the sidepanel and buttons to remain using red. The place that stuck out a bit for me is the bottom of extra pages:

New:
image

Old:
image

The Next Page/Previous Page links are red, while everything around them is blue. I'm no design/accesibility expert, so I'm happy to keep things as they are. Just wanted to double check that it was intended.

@josevalim
Copy link
Member

I see. Let's see what @hichemfantar has to say about it but, when looking at the Docusaurus, as an example, they keep it in the theme color, and not as a link.

@hichemfantar
Copy link
Contributor Author

I suppose i agree with that we should also use the link color in the pagination buttons.

@garazdawi any other places you wish to see this change?

@hichemfantar
Copy link
Contributor Author

hichemfantar commented Jan 27, 2025

or we can use the link theme colors i created here #2026 (comment)

they pass wcag checks

@josevalim
Copy link
Member

josevalim commented Jan 27, 2025

@hichemfantar do you mean having the links in red? We had this before and we received several complaints about the color as a whole, so I would not go down this path. :)

@josevalim
Copy link
Member

I can think of three options:

  1. Leave it as is
  2. Make the title of the next/prev page itself in link color
  3. Swap the colors of "Next page"/"Prev page" with the title, which is what docusauros does

@hichemfantar
Copy link
Contributor Author

alright i'll push a fix with 2 and 3

@DavidOliver
Copy link
Contributor

Were the complaints due to the the hue (red) outright, or to them lacking contrast against the background? I wonder if we could lighten/darken the red link colour as necessary? I haven't looked yet (just about to), but I imagine a mixture of red and blue links may look a little strange?

@josevalim
Copy link
Member

It was with the red outright.

@hichemfantar
Copy link
Contributor Author

i did some more testing and red is def bad for test so let's avoid it

@hichemfantar
Copy link
Contributor Author

#2075

@garazdawi
Copy link
Contributor

@garazdawi any other places you wish to see this change?

None that I have found yet.

@DavidOliver
Copy link
Contributor

It was with the red outright.

💔

@hichemfantar
Copy link
Contributor Author

should we also get rid of link-visited-color? I don't see the point in having it, it's useful in search engines to know u already visited a link but i don't know about docs. I don't think anyone else does it

@DavidOliver
Copy link
Contributor

Re. link-visited-color, I think accessibility buffs will say it's important, for some users more than others. I'm in two minds about it.

@hichemfantar
Copy link
Contributor Author

btw the erlang main color has bad contrast in dark mode while elixir main color had bad contrast in light mode

image

image

it's best to use tokens like these https://ui.shadcn.com/themes that operate much better in light dark situations although this would be a large change

@layer base {
  :root {
    --background: 0 0% 100%;
    --foreground: 240 10% 3.9%;
    --card: 0 0% 100%;
    --card-foreground: 240 10% 3.9%;
    --popover: 0 0% 100%;
    --popover-foreground: 240 10% 3.9%;
    --primary: 240 5.9% 10%;
    --primary-foreground: 0 0% 98%;
    --secondary: 240 4.8% 95.9%;
    --secondary-foreground: 240 5.9% 10%;
    --muted: 240 4.8% 95.9%;
    --muted-foreground: 240 3.8% 46.1%;
    --accent: 240 4.8% 95.9%;
    --accent-foreground: 240 5.9% 10%;
    --destructive: 0 84.2% 60.2%;
    --destructive-foreground: 0 0% 98%;
    --border: 240 5.9% 90%;
    --input: 240 5.9% 90%;
    --ring: 240 5.9% 10%;
    --radius: 0rem;
    --chart-1: 12 76% 61%;
    --chart-2: 173 58% 39%;
    --chart-3: 197 37% 24%;
    --chart-4: 43 74% 66%;
    --chart-5: 27 87% 67%;
  }

  .dark {
    --background: 240 10% 3.9%;
    --foreground: 0 0% 98%;
    --card: 240 10% 3.9%;
    --card-foreground: 0 0% 98%;
    --popover: 240 10% 3.9%;
    --popover-foreground: 0 0% 98%;
    --primary: 0 0% 98%;
    --primary-foreground: 240 5.9% 10%;
    --secondary: 240 3.7% 15.9%;
    --secondary-foreground: 0 0% 98%;
    --muted: 240 3.7% 15.9%;
    --muted-foreground: 240 5% 64.9%;
    --accent: 240 3.7% 15.9%;
    --accent-foreground: 0 0% 98%;
    --destructive: 0 62.8% 30.6%;
    --destructive-foreground: 0 0% 98%;
    --border: 240 3.7% 15.9%;
    --input: 240 3.7% 15.9%;
    --ring: 240 4.9% 83.9%;
    --chart-1: 220 70% 50%;
    --chart-2: 160 60% 45%;
    --chart-3: 30 80% 55%;
    --chart-4: 280 65% 60%;
    --chart-5: 340 75% 55%;
  }
}

@hichemfantar
Copy link
Contributor Author

use this tool for contrast checks

https://webaim.org/resources/contrastchecker/

@josevalim
Copy link
Member

@hichemfantar I think we are using the wrong color there. We already have a color we use in the sidebar for dark mode with better accessibility, but it was not reflected in the cheatsheets. Are you going to send a PR or should I look into it?

@josevalim
Copy link
Member

Actually, we should just get rid of the colors. I will push a fix to main.

@DavidOliver
Copy link
Contributor

DavidOliver commented Jan 27, 2025

@hichemfantar, apologies if I'm missing something, but is there something that that structure provides for that our :root- and body.dark-scoped custom properties (light and dark mode respectively), including the _elixir and _erlang CSS files with their five tones of the programming language colour, don't? As far as I know, we can control both light and dark mode colours fairly well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants