Skip to content

Add theming function#72

Merged
sbreitbart-NOAA merged 8 commits intomainfrom
ggplot2-update
Nov 25, 2025
Merged

Add theming function#72
sbreitbart-NOAA merged 8 commits intomainfrom
ggplot2-update

Conversation

@sbreitbart-NOAA
Copy link
Collaborator

Add NMFS-branded theming function, as per #71

@sbreitbart-NOAA sbreitbart-NOAA linked an issue Nov 18, 2025 that may be closed by this pull request
@github-actions

This comment has been minimized.

@sbreitbart-NOAA
Copy link
Collaborator Author

Hi @k-doering-NOAA ! Are you interested in / do you think you'll have capacity to review this PR within the next month or so? If not, I can ask someone else- I realize you're still acclimating back to work. Thanks!

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@k-doering-NOAA
Copy link
Contributor

@sbreitbart-NOAA, that works for me! I will finish by 12/5

@sbreitbart-NOAA
Copy link
Collaborator Author

@sbreitbart-NOAA, that works for me! I will finish by 12/5

Super! Thanks so much, Kathryn!

Copy link
Contributor

@k-doering-NOAA k-doering-NOAA left a comment

Choose a reason for hiding this comment

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

Hi Sophie,

Overall, it looks good! I was able to run the example plot that uses the theme! I just had a few minor comments for your consideration. They are all optional, if you decide to make no changes I think it is still ok to merge this.

One additional thing I wondered is if the theme should include the fonts recomended by the Fisheries style guide - However, I looked around a bit and it seemed like it could be a rabbit hole (e.g., this post) as it is difficult to know which fonts folks have on their computer. So maybe this isn't worth investigating further.

Thanks for putting this together, it seems like it will be a useful feature!

@sbreitbart-NOAA
Copy link
Collaborator Author

Hi Sophie,

Overall, it looks good! I was able to run the example plot that uses the theme! I just had a few minor comments for your consideration. They are all optional, if you decide to make no changes I think it is still ok to merge this.

One additional thing I wondered is if the theme should include the fonts recomended by the Fisheries style guide - However, I looked around a bit and it seemed like it could be a rabbit hole (e.g., this post) as it is difficult to know which fonts folks have on their computer. So maybe this isn't worth investigating further.

Thanks for putting this together, it seems like it will be a useful feature!

Thanks so much for reviewing this thoroughly and swiftly! You made some great points and I'll momentarily push some changes in response.
Re: the fonts: I think this is a great idea in theory. However, as you note, it could be a headache to implement in a way that nearly guarantees success for the user. I'm very cautious when it comes to fonts as I've had a lot of trouble with them in the past. If/when there comes a dependable solution, let's do it!

@github-actions
Copy link
Contributor

Code Metrics Report

Coverage Code to Test Ratio Test Execution Time
87.6% 1:0.3 5s

Code coverage of files in pull request scope (86.3%)

Files Coverage
R/ggplot2-scales.R 93.8%
R/nmfs_cols.R 71.4%

Reported by octocov

@sbreitbart-NOAA sbreitbart-NOAA merged commit 222ba43 into main Nov 25, 2025
12 checks passed
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.

Add NMFS-branded ggplot2 themes

2 participants