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.

Sign up for GitHub

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

Jump to bottom

Custom plots vignette #162

Merged
merged 9 commits into from
Feb 21, 2025
Merged

Custom plots vignette #162

merged 9 commits into from
Feb 21, 2025

Conversation

sbreitbart-NOAA
Copy link
Collaborator

What is the feature?

How have you implemented the solution?

  • In a vignette

Does the PR impact any other area of the project, maybe another repo?

  • No

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 12, 2025 •
edited by sbreitbart-NOAA
Loading

Instructions for code reviewer

Hello reviewer, thanks for taking the time to review this PR!

  • Please use this checklist during your review, checking off items that you have verified are complete!
  • For PRs that don't make changes to code (e.g., changes to README.md or Github actions workflows), feel free to skip over items on the checklist that are not relevant. Remember it is still important to do a thorough review.
  • Then, comment on the pull request with your review indicating where you have questions or changes need to be made before merging.
  • Remember to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do.
  • PR reviews are a great way to learn, so feel free to share your tips and tricks. However, for optional changes (i.e., not required for merging), please include nit: (for nitpicking) before making the suggestion. For example, nit: I prefer using a data.frame() instead of a matrix because...
  • Engage with the developer when they respond to comments and check off additional boxes as they become complete so the PR can be merged in when all the tasks are fulfilled. Make it clear when this has been reached by commenting on the PR with something like This PR is now ready to be merged, no changes needed.

Checklist

  • The PR is requested to be merged into the appropriate branch (typically main)
  • The code is well-designed.
  • The functionality is good for the users of the code.
  • Any User Interface changes are sensible and look good.
  • The code isn’t more complex than it needs to be.
  • Code coverage remains high, indicating the new code is tested.
  • The developer used clear names for everything.
  • Comments are clear and useful, and mostly explain why instead of what.
  • Code is appropriately documented (doxygen and roxygen).

@sbreitbart-NOAA sbreitbart-NOAA linked an issue Feb 12, 2025 that may be closed by this pull request
Find methods to add alt text and captions for figures and table generated by user (so they can use the add_alttext fxn for it as well) #156
Closed
@sbreitbart-NOAA sbreitbart-NOAA marked this pull request as ready for review February 12, 2025 20:46
Schiano-NOAA
Schiano-NOAA reviewed Feb 12, 2025
View reviewed changes
vignettes/custom-figs-tabs.Rmd Outdated

## Saving text

Once written, you'll have to save your captions and alt text. While you could save it in the captions_alt_text.csv file (created if you've already generated a figure or table rda), we recommend saving these in a separate file to avoid loss if the captions_alt_text.csv file is overwritten.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about this a little more, maybe we can set the fxn that generates this csv to append/replace the rows it matches rather than overwrite?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's an idea that should eliminate overwriting risks: I set up write_captions to check for an existing captions_alt_text.csv file; if absent, then it creates one. Right now, it creates one (overwrites an existing one) if it's called (which shouldn't happen in a typical workflow, but it's possible).
Otherwise, the captions_alt_text.csv shouldn't be overwritten. It's only called again in add_more_key_quants, which intakes the captions_alt_text.csv as a df, pulls out one row (specific to the table or figure it's associated with), changes it by substituting more key quantities, replaces the old row with the new row, and then reexports the df to captions_alt_text.csv so that only that specific row is changed.
If you're on board with this idea, I can change the satf::write_captions fxn as described, and update this workflow so that they add their captions/alt text to their existing captions_alt_text.csv file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Works for me!

Schiano-NOAA
Schiano-NOAA requested changes Feb 12, 2025
View reviewed changes
Copy link
Collaborator

@Schiano-NOAA Schiano-NOAA left a comment

Choose a reason for hiding this comment

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

I think this is a great start to this vignette. I would like to talk about this more during our meeting tomorrow because I can foresee some issues/confusion with the workflow. The vignette also does not address custom figures/tables that are read in as PNGs or if users wanted to code directly into an R chunk in the quarto doc.

I think the vignette could benefit from a step-by-step overview of the workflow with hyper links to each of the sections below it.

@sbreitbart-NOAA
Copy link
Collaborator Author

I think this is a great start to this vignette. I would like to talk about this more during our meeting tomorrow because I can foresee some issues/confusion with the workflow. The vignette also does not address custom figures/tables that are read in as PNGs or if users wanted to code directly into an R chunk in the quarto doc.

I think the vignette could benefit from a step-by-step overview of the workflow with hyper links to each of the sections below it.

Thanks! I'll put it on the agenda for our meeting. I'll also explicitly add that this workflow will be of use if users want to add already-made plots (e.g., PNG) and how users can directly code a plot into a quarto doc. I can also add a step-by-step overview with some more detail than is covered in the table of contents, which will contain the links to heading 1 and 2 sections.

Schiano-NOAA
Schiano-NOAA approved these changes Feb 20, 2025
View reviewed changes
Copy link
Collaborator

@Schiano-NOAA Schiano-NOAA left a comment

Choose a reason for hiding this comment

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

I like it! It's long but the detail is needed. The only thing it needs is the TOC in a sidebar so people can better navigate - which I know we can't figure out why that's not working

@sbreitbart-NOAA sbreitbart-NOAA merged commit 7e7435b into dev Feb 21, 2025
@sbreitbart-NOAA sbreitbart-NOAA deleted the custom-plots-vign branch February 21, 2025 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Schiano-NOAA Schiano-NOAA Schiano-NOAA approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@sbreitbart-NOAA @Schiano-NOAA