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

remove unused race categories from legend #143

Merged
merged 5 commits into from
Feb 7, 2023
Merged

remove unused race categories from legend #143

merged 5 commits into from
Feb 7, 2023

Conversation

leotorres114
Copy link
Collaborator

Decided to try to implement Noel's first approach from Issue 142, which seems a bit more intuitive to the end user, e.g. provide which race categories are available the data and cmap_fill_race() / cmap_color_race() should automatically exclude those not provided.

match.call() was used because cmap_fill/color_race is passing all of its arguments to make_race_palette, which uses that list of arguments to build the palette. if match.call() wasn't used, the arguments passed to make_race_palette would be unevaluated symbol class (better explained here), which wouldn't allow make_race_palette to build a palette from the arguments passed to it. Most resources pointed to the need to use match.call() for this scenario, but maybe there's a better way to address this?

do.call() is just executing the make_race_palette with a list of the arguments (names and values) provided to cmap_fill/color_race().

To build the palette, make_race_palette loops through the names of race_palette - if that name is included in the names of the argument list passed, it adds the argument list value of passed as the index and hex value of race_palette as the value to an empty character vector, pal.

@leotorres114 leotorres114 added the bug Something isn't working label Jul 19, 2022
@leotorres114 leotorres114 self-assigned this Jul 19, 2022
@leotorres114 leotorres114 linked an issue Jul 19, 2022 that may be closed by this pull request
reflects changes made which address issue 142
R/colors_race.R Outdated Show resolved Hide resolved
R/colors_race.R Show resolved Hide resolved
R/colors_race.R Outdated Show resolved Hide resolved
R/colors_race.R Show resolved Hide resolved
Copy link
Collaborator

@dlcomeaux dlcomeaux left a comment

Choose a reason for hiding this comment

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

When the code is done, please also update documentation (using the RStudio tools to do that, Ctrl + Shift + D should do the trick) and make any necessary updates to the vignettes. Thanks!

Copy link
Collaborator

@dlcomeaux dlcomeaux left a comment

Choose a reason for hiding this comment

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

And, we'll want to update the version number and note the changes in News.md

see Daniel's comments in PR #143
@leotorres114
Copy link
Collaborator Author

@dlcomeaux documentation, vignettes, and News.md have been updated accordingly. Let me know if you have any thoughts about my reply to your comment above.

Changes based on Roxygen - not substantive
@dlcomeaux
Copy link
Collaborator

@dlcomeaux documentation, vignettes, and News.md have been updated accordingly. Let me know if you have any thoughts about my reply to your comment above.

Sorry for the very long delay. This solution works, and I agree, seems more trouble than it's worth to go after a more complex solution.

@dlcomeaux
Copy link
Collaborator

@leotorres114 this works, I will update documentation and merge.

@dlcomeaux
Copy link
Collaborator

@EthanJantz @amcadamsCMAP question for you - this is failing in the pkgdown part of the run, with an error that I don't recognize. Are either of you familiar with what might cause this?

@leotorres114
Copy link
Collaborator Author

@dlcomeaux it looks like the gh actions workflow file is outdated. it can't find r-lib/actions@master because it's been updated (reference). I created another PR to update the workflow file, #145 , with the correct version. updating that first should (hopefully) take care of that error.

@dlcomeaux dlcomeaux merged commit 75b6eab into master Feb 7, 2023
@dlcomeaux dlcomeaux deleted the issue-142 branch February 7, 2023 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unused race/ethnicity categories from legend
2 participants