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

504 consistent color pairs for outlinefill for figures #507

Merged

Conversation

wokenny13
Copy link
Collaborator

Color pairings were done within the TADA_ColorPalette function using an optional argument. Reflected these changes in some of the .R Figures where it seemed appropriate.

@wokenny13 wokenny13 linked an issue Aug 6, 2024 that may be closed by this pull request
@hillarymarler
Copy link
Collaborator

@wokenny13 - I will review this PR tomorrow or Friday.

Copy link
Collaborator

@hillarymarler hillarymarler left a comment

Choose a reason for hiding this comment

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

@wokenny13 - this was a really nice and efficient solution. What would you think of adding the col_pair argument to the TADA_ViewColorPalette function? So that users/devs could easily preview the color pairs. Could be done as part of this PR or a future one.

@wokenny13
Copy link
Collaborator Author

I think that's something that I would be able to add on to TADA_ViewColorPalette as part of this pr. I can work on completing that item either later today or early next week

@hillarymarler
Copy link
Collaborator

Thanks - that sounds like a great plan. I will wait to merge until you have had a chance to add that feature.

@wokenny13
Copy link
Collaborator Author

wokenny13 commented Aug 12, 2024

@hillarymarler I added the col_pair as a view for TADA_ViewColorPalette and it is ready for review. I created the output as a matrix, but am open to suggestions on how it should be viewed if there are other thoughts.

For the example:

#' TestViewPalette <- TADA_ViewColorPalette() #' TestViewPalettePairing <- TADA_ViewColorPalette(col_pair = TRUE)

I noticed that if you run it, and try to store it under the variable, for example 'TestViewPalette', that TestViewPalette returns NULL as the value. It still, initially, plots the view color palette function output, but if you try to call the variable it was stored under, 'TestViewPalette' only NULL is returned. Not sure if that's expected or not.

I think the update that I made, plots both the original TADA_ViewColorPallete and the paired figures output, in separate plots. If you feel like it should only plot either the paired output or the original figure, depending on if col_pairs = TRUE/FALSE, I can fix that.

@hillarymarler
Copy link
Collaborator

hillarymarler commented Aug 13, 2024

This looks great. I like that way you have the output set up. What would you think about swapping out tada palette #4 for a pair from 12/13/14 from the unpaired palette? To make sure they are all distinct enough.

image

Utilized TADA_CHarStringRemoveNA for y axis title

color changed palette 4 pairing

Tried to add legend title. Not sure if a legend title should be used or not?
@@ -1380,23 +1376,23 @@ TADA_GroupedScatterplot <- function(.data, group_col = "MonitoringLocationName",
plot_bgcolor = "#e5ecf6",
margin = mrg,
legend = list(
title = list(text = paste0('<b>', group_col,'<b>'), x = 0.5, y= 100),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hillarymarler over here for legend title

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

@hillarymarler hillarymarler merged commit 848d8c5 into develop Aug 16, 2024
7 checks passed
@hillarymarler hillarymarler deleted the 504-consistent-color-pairs-for-outlinefill-for-figures branch August 16, 2024 18:03
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.

Consistent color pairs for outline/fill for figures
2 participants