Serubin / pulse-sms-web

The official web app for Pulse SMS - built on Vue.js.
https://pulsesms.app
Other
109 stars 44 forks source link

Fix Text Color to Meet WCAG 2.0 Contrast Minimum #108

Closed sstream17 closed 4 years ago

sstream17 commented 4 years ago

Currently the text color is not possible to see for some colors and themes.

For example, when the color is too bright, the text will appear white and it is very hard to read. image

This change sets the project to follow the WCAG 2.0 Contrast Minimum when setting text colors.

The same color now sets the text to be black. image

I have followed the guideline recommendation of setting the ratio to be 4.5:1.

Serubin commented 4 years ago

This looks awesome. I wasn't able to reproduce the issue with that particular green color, but I've definitely seen this be an issue.

I added a couple of nitpicks - should be good to merge once we resolve those.

Thanks for submitting this!

klinker24 commented 4 years ago

Sorry it took awhile for me to look at this, but I do have some thoughts around it. I am also holding off deploying this until I can get Android prepared with the same changes.

While I definitely agree that improvements can and should be made in this area, I honestly think that 4.5:1 is too much. For example, it marks this color as needing dark text, which I don't agree with and think that just looks bad:

Screen Shot 2020-02-20 at 2 34 12 PM

In my opinion, using 0.3 is a better number than 0.233. That will, at least, make this message use white text. Obviously this is completely opinion based, but it is my name on the app listing and me that has to deal with people complaining about changes 🙈frankly, I will get complaints about a UI change like this. This change is pretty drastic and seems to affect tons of different colors/conversations

sstream17 commented 4 years ago

In my opinion, using 0.3 is a better number than 0.233

I agree! Feel free to change the value to anything you prefer. I set the initial ratio to 4.5:1 because that is what the guideline recommended. I did notice that many reds displayed black text using that ratio and I did think it was more difficult to see them.

Serubin commented 4 years ago

Apologies - jumped the gun on the merge

klinker24 commented 4 years ago

No worries, usually I am much faster at getting to PRs. I was just slower on this one