Inject accessibility #124
Inject accessibility #124
Conversation
….. only satf. Then updated a11y vignette to reflect where users should look for the file.
…p chunk to false, re: #102 . Will change eval to true for all chunks once indices table fxn is completed.
…per code for testing
Instructions for code reviewerHello reviewer, thanks for taking the time to review this PR!
Checklist
|
…led both using the add_accessibility function Also adjusted testing function for add_alttext and removed old snapshot - was not working properly
|
@sbreitbart-NOAA I made the changes as we discussed and tested if they work. The testing function isn't working exactly how I want so it is not finished, but the functions themselves should be good to go. |
Sounds great, thanks! |
* Remove nmfs-pallete from dependencies temporarily * Comment out reference to nmfs pallete in add_theme and tests
Co-authored-by: Schiano-NOAA <[email protected]>
-Fix so that rda files can be added as input, if already created, OR generated if model results are included as an argument. -Update documentation
…ociated files and changes to create_template (#152)
… acknow, and tables and figs if desired
….. only satf. Then updated a11y vignette to reflect where users should look for the file.
* Remove nmfs-pallete from dependencies temporarily * Comment out reference to nmfs pallete in add_theme and tests
* Minor grammatical changes just to set up new dev-1.1 branch (changed quarto --> Quarto) * Feat(vignettes): -added tables of contents to vignettes -added tip for how to edit rda files to a11y vignette -added link to faqs vignette to a11y vignette re: how to edit alt text and captions --------- Co-authored-by: sbreitbart-NOAA <[email protected]>
* update(create_template): comment out skeleton pop up at the end * update(add_section): add functionality for putting new subsection into section * Update(add_section): fix pathing for locating section to place code and change from add_chunk to add_child * update(create_template): select only custom_sections when user wants to make a different outline of report custom must = TRUE otherwise copies all files * Fix(create_template): adjust if statement argument for custom_sections * Add nmfspalette package back in * removed add_theme and associated test and documentation * remove static NOAA Fishies in authorship * update NOAA fisheries affiliations Co-authored-by: Schiano-NOAA <[email protected]> --------- Co-authored-by: sbreitbart-NOAA <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Schiano-NOAA <[email protected]>
…led both using the add_accessibility function Also adjusted testing function for add_alttext and removed old snapshot - was not working properly
* Remove nmfs-pallete from dependencies temporarily * Comment out reference to nmfs pallete in add_theme and tests
… acknow, and tables and figs if desired
…igures inserted into the outline (png)
…ither from images or code chunks
|
@sbreitbart-NOAA this is about ready to merge. I will fix a couple of errors being thrown in testing on thursday, but I want to get this out. The tests for the functions are not complete. Could you specifically test the add_tagging function? I keep ending up with the latex package issues as before and it makes no sense. Thanks! |
There was a problem hiding this comment.
Most of my comments are minor and can be addressed quickly. I really like how there are separate functions for adding alt text and tagging, too, and that the user is messaged when steps are completed! I have two larger issues I can't solve, though:
- I can't get add_accessibility to work
- I can't verify whether compiling works for the smaller fxns too:
- For add_tagging(), compiling gives this error message: "! LaTeX Error: File `accessibility.tex' not found." I'm not sure if this error is arising because of my mistakes, or because of function issues, because that .tex file is created and saved in the report folder.
- For add_alttext, compiling gives this error message: "! Package luatex.def Error: File `support_files/Petrale_sole.png' not found: using draft setting." This occurs even though Petrale_sole.png is in the report/support_files directory. Again, is this an error on my part, maybe due to tinytex installation issues?
R/add_accessibility.R
Outdated
| @@ -8,11 +8,11 @@ | |||
| #' @param compile TRUE/FALSE - indicate whether the document (X) should be rendered after these files are changed | |||
| #' @param rename change the name of the latex file for final compilation or saving | |||
| #' | |||
| #' @return DRAFT: This function was made to help add in latex packages and content | |||
| #' associated with PDF tagging. Quarto does not allow the user to edit anything | |||
| #' before documentclass, so this function alters the rendered .tex file. Users | |||
There was a problem hiding this comment.
Please clarify what these arguments mean and what they could look like. I had to look into the source code to figure out what some of the args were actually asking for. For instance, alttext_csv_dir requires a directory + the csv name, whereas the other dir-related args (dir, rda_dir) only require a directory. Similarly, I wasn't sure if rename required ".tex" or not. So, I'd make the following changes:
- Change
dirtotex_dir, for clarity - Change the arg
alttext_csv_dirtoalttext_csv - Change the
alttext_csvarg definition to state that this references the location of the alt text csv - Update the
renamedefinition to specify that this new name shouldn't include the .tex filetype - Add the default value of
compileto its definition - Update the
xdefinition to specify that this is the .tex file into which alt text, not "accessibility", is added - Make the "X" within the
compiledoc lowercase to match thexargument
R/add_alttext.r
Outdated
| #' | ||
| #' @inheritParams add_accessibility | ||
| #' | ||
| #' @return DRAFT: This function was made to help add in |
There was a problem hiding this comment.
Is this still a draft?
There was a problem hiding this comment.
This is and is outdated somehow after your review which makes no sense because I pushed all of these changes on tuesday
R/add_tagging.r
Outdated
| #' | ||
| #' @inheritParams add_accessibility | ||
| #' | ||
| #' @return DRAFT: This function was made to help add in |
There was a problem hiding this comment.
Is this still a draft? (for this and the other 2 identical comments, please remove "DRAFT: " if it's ready to merge)
| @@ -0,0 +1,122 @@ | |||
| #' Add alternative text into latex | |||
There was a problem hiding this comment.
Minor: please change the filetype from .r to .R for consistency
| @@ -0,0 +1,56 @@ | |||
| #' Add tagging structure to latex documents produced from quarto | |||
There was a problem hiding this comment.
Minor: please change the filetype from .r to .R for consistency
| #' function as a way to add alternative text to all | ||
| #' figures, please leave an issue to request this function. | ||
| #' | ||
| #' @export |
There was a problem hiding this comment.
Please add examples if they're absent (I can see an example in Rstudio but not here... ?), and update the example to include the alttext_csv_dir and rename args.
There was a problem hiding this comment.
Added on tuesday...not sure why none of my changes are there. Git is telling me that everything is updated
| #' @return DRAFT: This function was made to help add in | ||
| #' latex packages and content associated with PDF | ||
| #' tagging. Quarto does not allow the user to edit anything | ||
| #' before documentclass, so this function alters the rendered .tex file. |
There was a problem hiding this comment.
Also don't see the example here, but I do see it in my Rstudio file ...? Please update the example to include the fxn's rename arg (currently includes rda_dir instead, which isn't an add_tagging() arg)
There was a problem hiding this comment.
This function isn't working for me. I'm using the same args as for add_tagging and add_alttext, but I get an error I think is about incorrect filepaths ("Error in file(file, "rt") : cannot open the connection"). The error arises at line 73, when add_alttext is called.
| @@ -1,10 +1,12 @@ | |||
| # Generated by roxygen2: do not edit by hand | |||
There was a problem hiding this comment.
Also, meant to mention this: Initially, I had some issues rendering the 08_tables.qmd. Based on conversations from a couple of weeks ago, I'm sure this will be resolved with rebasing (these errors arose for you before, I believe, as explained here: #153 )
What is the feature?
*Create a function to increase document accessibility
How have you implemented the solution?
*Create a function, documentation, and testing to edit the latex produced template to add alternative text and tagging using \tagpdf function before the document class structure. The tagging portion will propably be deprecated in the future with the addition of a new partial in the template
Does the PR impact any other area of the project, maybe another repo?
*no, the function should be used at the end of the stock assessment workflow