-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Use WCAG recommendation to fill in GTFS route text color if it is missing #6308
Use WCAG recommendation to fill in GTFS route text color if it is missing #6308
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6308 +/- ##
=============================================
+ Coverage 69.79% 69.81% +0.02%
- Complexity 17798 17827 +29
=============================================
Files 2019 2021 +2
Lines 76126 76248 +122
Branches 7786 7802 +16
=============================================
+ Hits 53132 53234 +102
- Misses 20288 20299 +11
- Partials 2706 2715 +9 ☔ View full report in Codecov by Sentry. |
I have not read the code, but from the title, is this inserting data into the OTP model of it is missing in the feed? We do not do that in general. OTP should plan, not correct incomplete data. |
Yes, it is adding missing data in the feed. This code has already been there for 9 years. Do you think we should remove it? I am happy to remove this piece of code as well. |
Ok if it already exists then this is probably å good fix. We can talk about it next week |
OK let's discuss if we should
|
utils/src/test/java/org/opentripplanner/utils/color/ColorUtilsTest.java
Outdated
Show resolved
Hide resolved
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.
Generally I'm ok with this, since it replaces one type of guesswork with another one.
I have a comment about the test, so please take a look.
I am ok with this as well. I think it removes guesswork with something that is better, the WCAG recommendation is after all a W3C Guideline. |
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.
This looks fine to me. As @t2gran said, this is replacing some existing guesswork with newer guesswork that we seem to all agree yields a better result. My only concern was that readers might be left wondering whether the WCAG recommendations hold some special status in the OTP project, whether they should start applying or implementing those standard elsewhere in the project. A note to that effect, even on this PR could help someone in the future.
I wasn't even aware that this functionality exists. I think we would be hesitant to add this as a default functionality now but since this has already existed for a long time, I don't know if this is something we can get rid of or make default off anymore. Main problem with this code is that it makes it difficult to know if some color is part of some brand or is it something we generate (i.e. making it more difficult for a client to decide if this color should be used or not). |
Summary
This is a rework of #5490 . It uses the WCAG recommendation to decide if the text color should be black (on a light background) or white (on a dark background).
The color calculation is refactored out into a separate util module such that it is testable, and is changed to use the official sRGB color space formula.
The luminance threshold is 0.179 according to the WCAG formula (where the previous code used 0.5). It has an effect of considering red (255, 0, 0) a light color that black text (instead of white) should be placed on red background, as black has a greater contrast with red than white per accessibility guidelines. This will need some discussion.
Issue
None
Unit tests
Added for color calculation.
Documentation
Javadoc