Skip to content

Friendly error links have poor color contrast #2046

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

Jump to bottom
Closed
stampyzfanz opened this issue Jul 30, 2022 · 11 comments · Fixed by #2156
Closed

Friendly error links have poor color contrast #2046

stampyzfanz opened this issue Jul 30, 2022 · 11 comments · Fixed by #2156
Labels
Area:Accessibility Category for accessibility related features and bugs Bug Good First Issue

Comments

@stampyzfanz
Copy link

Details about the bug:
@welcome
Copy link

welcome bot commented Jul 30, 2022

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, be sure to follow the issue template if you haven't already.

@stampyzfanz stampyzfanz changed the title Friendly error links have poor text contrast Friendly error links have poor color contrast Jul 30, 2022
@kjhollen kjhollen added the Area:Accessibility Category for accessibility related features and bugs label Aug 15, 2022
@Brady-Edwards
Copy link

Hi there, if someone isn't already working on this, I'd like to have a crack! This would also be my first open source contribution :).

@stampyzfanz
Copy link
Author

I am not aware of anyone working on it and feel free to let me know if there's anything I can do to help.

Thanks for your interest :)

@Brady-Edwards
Copy link

I've got a working fix, can a maintainer give me the green light to open a PR? Thanks.

@raclim
Copy link
Collaborator

raclim commented Nov 9, 2022

Hi @Brady-Edwards, super excited to see your work! :) Please feel free to go through with the PR, or let me know if there's anything else you need beforehand!

@devtarun
Copy link

devtarun commented Jan 7, 2023

Hello. is this still open?

@ayushbhaimehta
Copy link

Hi there. If this haven't been resolved yet then I would love to do so. Also can somebody point me where can I get slack or discord link where there is more discussion on issues.
Thank you

@raclim
Copy link
Collaborator

raclim commented Jan 18, 2023

Hi @devtarun and @ayushbhaimehta! Yes, this is still currently open, but I think @Brady-Edwards had some work done on this earlier. However, please feel free to add to this issue!

This is the invite URL for the p5.js discord. :)

@Brady-Edwards
Copy link

Hi there, I had completely forgotten about this (got carried away with life)! I had already pin-pointed the issue and a fix for it but never got around to issuing a pull request. However, I'm more than happy for someone else to issue the pull request for this instead :). Either way, I'll quickly detail what I found to cause the issue here:

In this snippet here, the color and fill lines are getting overridden with some external css (not sure what's the cause for the overwrite, but it doesn't seem to be from a css stylesheet in the repository). To prevent these from being overridden, the !important tag can be used, which seemed to fix the bug for me. I'm sure there's a better fix out there, but this is all I could find from the time I spent looking at this.

@ayushbhaimehta
Copy link

@raclim I pretty much agree with brady. The CSS seemed to be overridden by 2-3 external modules of CSS (one is definitely from main but still can't figure out the other ones).

@ayushbhaimehta ayushbhaimehta mentioned this issue Jan 18, 2023
Closed
@lindapaiste
Copy link
Collaborator

I looked into this a bit so I want to get this written down for posterity. The TL;DR is that we are missing an important prop on a component from an external package.

Here's what I'm seeing in dev tools
image

There is an appropriate link color #f0f0f0 that is set for all links when in .contrast, but that gets overwritten by another style which sets the link color for the console. That's the Emotion-generated CSS .css-kidsne a. The console element <div data-type="string" class="css-e393sm"> is a closer ancestor than the <body class="contrast"> so that style will get priority.

The console component comes from an external package 'console-feed', but we are passing in quite a lot of styling information to customize it.

What we are not doing, however, is passing a variant prop to the <ConsoleFeed/> component! The console-feed package supports light and dark modes, and the default styling will change based on which you select. You can see that in their source code here:

LOG_LINK_COLOR: isLight ? 'rgb(66, 66, 66)' : 'rgb(177, 177, 177)',

There are two ways to fix this:

  1. We can pass a variant="dark" prop to the <ConsoleFeed/> when in "dark" or "contrast" modes. This changes the default color from the unreadable dark gray rgb(66, 66, 66) to a light gray rgb(177, 177, 177)

Contrast:
image
Dark:
image

We need to add one additional line to the code here

<ConsoleFeed
    variant={theme === "light" ? "light" : "dark"} // <---- this line
    styles={getConsoleFeedStyle(theme, fontSize)}
    logs={consoleEvents}
    key={`${theme}-${fontSize}`}
/>
  1. We can define our own LOG_LINK_COLOR as whatever precise color we want it to be. We would add an additional property to the CONSOLE_FEED_DARK_STYLES and CONSOLE_FEED_CONTRAST_STYLES objects.

I recommend #1 for simplicity.

nown1ne added a commit to nown1ne/p5.js-web-editor that referenced this issue Mar 8, 2023
@nown1ne
Readability of links in console
fbc01fd 
Based on the suggestion of @lindapaiste here processing#2046 (comment)
I have made the changes and testes them locally.
@nown1ne nown1ne mentioned this issue Mar 8, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area:Accessibility Category for accessibility related features and bugs Bug Good First Issue
Projects
None yet
Milestone
No milestone
7 participants
@kjhollen @devtarun @lindapaiste @stampyzfanz @raclim @ayushbhaimehta @Brady-Edwards