-
Notifications
You must be signed in to change notification settings - Fork 212
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
Autocoloroptions #1930
base: master
Are you sure you want to change the base?
Autocoloroptions #1930
Conversation
Hi @rich-iannone and @olivroy since it has been a while since I opened this pull request, and I would still like to be able to use this feature in various situations (i.e., tables I create), I would like to ask what you think about it!? |
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.
Overall, good start! Sorry for the delay.
I think this is a good idea. Can't garantee that we merge it. I'd suggest adding some before / after (visual) examples of the improvement this PR brings.
We'd like to avoid new imports, so if you find a way to accomplish this without using farver, it would be great.
It is good that you added some tests.
Also, feel free to use gt from your PR `pak::pak("#1930") and report back if you see that something that is off.
You would need to add a news bullet as well that briefly describe the change https://style.tidyverse.org/news.html
DESCRIPTION
Outdated
@@ -38,6 +38,7 @@ Imports: | |||
cli (>= 3.6.3), | |||
commonmark (>= 1.9.1), | |||
dplyr (>= 1.1.4), | |||
farver, |
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.
Could you add this in Suggrsts instead? We cannot add another import due to CRAN restrictions
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.
Understand. Using {farver} to offer the user the possibility to specify the color in a range of different specifications (literally everything that {farver} can handle), as well as {farver} has the task and responsibility to convert precisely (remember also base R limitations) into the desired #RRGGBB hexadecimal format.
I have added it to “Suggests”, as has been done with other pkgs elsewhere in gt with packages that are only used in a very specific case. I think that is reasonable.
R/data_color.R
Outdated
@@ -233,6 +234,22 @@ | |||
#' default this is `"apca"` (Accessible Perceptual Contrast Algorithm) and the | |||
#' alternative to this is `"wcag"` (Web Content Accessibility Guidelines). | |||
#' | |||
#' @param autocolor_light |
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.
Please follow the pattern above and give a short description
For example, #' @param autocolor_light *Automatic light color*
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.
done
R/data_color.R
Outdated
#' `"white"` will be used (`#FFFFFF"`). Alpha channel values will be set to | ||
#' 1.0 (fully opaque). | ||
#' | ||
#' @param autocolor_dark |
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.
Same
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.
done
light_color <- farver::encode_colour(farver::decode_colour(light)) | ||
dark_color <- farver::encode_colour(farver::decode_colour(dark)) |
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.
Why is this necessary?
We don't use this pattern of encoding / decoding colors in the package..
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.
See just above.
rgb[] <- ifelse(rgb <= 0.03928, rgb / 12.92, ((rgb + 0.055) / 1.055)^2.4) | ||
rgb[] <- ifelse(rgb <= 0.04045, rgb / 12.92, ((rgb + 0.055) / 1.055)^2.4) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Here's an example, not because it is particularly aesthetically pleasing, but I think it serves the purpose of clearly pointing out differences. Run it with and without
|
I have added light and dark
autocolor_text
color options. Often, tables are embedded in a context and in there "black" is not black, but another very dark color and analogously for white. Being able to design the tables in harmony is central.The current implementation follows the previous design of {gt}, particularly
data_color()
, but/thus it is unattractive in that ifautocolor = FALSE
, now allready 3 arguments are irrelevant.testthat
unit tests totests/testthat
for any new functionality.Additionally, I fixed a constant in the calculation of relative luminance WCAG (according to documentation).