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

wbColor #292

Merged
merged 16 commits into from
Sep 21, 2022
Merged

wbColor #292

merged 16 commits into from
Sep 21, 2022

Conversation

JanMarvin
Copy link
Owner

Provide wbColor. At the moment this requires a bit more testing. The main feature is, a general color class. This improves handling indexed, theme and rgb colors as well as validated colors.

wb <- wb_workbook() %>% 
  wb_add_worksheet("S1") %>% 
  wb_add_data("S1", mtcars) %>% 
  wb_add_fill(dims = "D5:J23", color = wb_color("blue")) %>% 
  wb_add_font(dims = "D5:J23", color = wb_color("gray")) %>% 
  wb_add_fill(dims = "E5:E23", color = wb_color("gray")) %>% 
  wb_add_fill(dims = "F5:F23", color = wb_color("gray", tint = 0.6)) %>% 
  wb_add_fill(dims = "G5:G23", color = wb_color(theme = "6")) %>% 
  wb_add_fill(dims = "H5:H23", color = wb_color(theme = "11")) %>% 
  wb_add_fill(dims = "I5:I23", color = wb_color(indexed = "4")) %>% 
  wb_open()

@JanMarvin
Copy link
Owner Author

Maybe you can have a look @jmbarbone ? If you're short on time, a hint if you like it or not will do. This is a breaking change that introduces wb_color(), to simplify handling of colors.

If we want to add this, we might need a few more if (!is.null(color)) assert_color(color) to be on the safe side. And it's a bit inconsistent, where we apply colors and where not, but that might be possible to iron out in a follow up PR.

@JanMarvin JanMarvin changed the title Wb color wbColor Aug 1, 2022
@jmbarbone
Copy link
Collaborator

@JanMarvin I'll be able to take a peak this week.

Quick thoughts: Is wbColor needed as an {R6} object? Looks mostly like a named list with a single xml related method. Would there be some plans to extend the class any more?

@JanMarvin
Copy link
Owner Author

Not really, I just to inspiration from the wb_hyperlink class, which does not really do anything either :)
If some simpler S3 function works better for you, I can switch over to that as well.

@jmbarbone
Copy link
Collaborator

That makes sense. When I converted all the ReferenceClass objects to {R6} I wasn't checking if they could be simplified. Wasn't sure how each object was being used. I think S3 would make a bit more sense and be a bit more lightweight.

R/class-color.R Outdated Show resolved Hide resolved
R/class-color.R Outdated Show resolved Hide resolved
R/wb_styles.R Outdated
@@ -873,12 +877,12 @@ set_cell_style <- function(wb, sheet, cell, value) {
create_dxfs_style <- function(
font_name = "Calibri",
font_size = "11",
font_color = c(rgb = "FF9C0006"),
font_color = wb_color(rgb = "FF9C0006"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to use a default font color in options() like we have for options(openxlsx2.borderColor)?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm a bit undecided.

  1. I really really dislike how the function calls look, if they have all the options(openxlsx2.xxx) calls. Changing things under the hood, if not NULL seems like a bad idea too.
  2. Maybe we could go with a name = "default" arg. That will call the options(openxlsx2.borderColor) under the hood? But than again, we'd have to make sure that everywhere a "default" option is possible. That easily turns into a plethora of changes.

R/wb_styles.R Outdated Show resolved Hide resolved
@JanMarvin JanMarvin mentioned this pull request Sep 9, 2022
8 tasks
@JanMarvin
Copy link
Owner Author

Looong rebase.

  • Renamed it to wb_colour(). If CI is fine it'll get a NEWS bulletin and a merge.
  • The color/colour thing (Use color #307) can wait. This will need a few more changes, to provide color and colour functions and arguments for every function.
  • For now it's just a simple helper getting the needed xml color definitions. Therefore no breakage.

If we like this approach, we can go deeper in the future, prepare default options and asserts. But that's for future releases/rainy days.

@JanMarvin
Copy link
Owner Author

There are a few inherits, that can be removed. Otherwise we enforce wb_colour() which was my idea some time ago.

@JanMarvin
Copy link
Owner Author

well ... needs more fixes. 😞

@JanMarvin JanMarvin merged commit 3c1637a into main Sep 21, 2022
@JanMarvin JanMarvin deleted the wbColor branch September 21, 2022 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants