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

inline figures should not be converted to figures #445

Closed
zkamvar opened this issue Apr 26, 2023 · 1 comment · Fixed by #446
Closed

inline figures should not be converted to figures #445

zkamvar opened this issue Apr 26, 2023 · 1 comment · Fixed by #446
Assignees
Labels
bug Something isn't working frequency: low indicator that a use-case has a low-frequency in lessons

Comments

@zkamvar
Copy link
Contributor

zkamvar commented Apr 26, 2023

EDIT: this is actually an issue with the fact that inline figures should not be converted to figures, but rather, they should remain images.

There is an issue whereby someone may want to add a link to an image or badge so that if someone clicks on an image, they are transported to another website.

This is an established practice as documented in the MDN docs

from carpentries/lesson-transition#65 (comment), @ostephens writes:

In Setup the link to Binder using the "My Binder" image with markup

[![Binder](https://mybinder.org/badge.svg)](https://mybinder.org/v2/gh/betatim/openrefineder/6ba108b?urlpath=%2Fopenrefine)

results in image being rendered by not linked. The HTML generated from the markup is

<figure href="https://mybinder.org/v2/gh/betatim/openrefineder/6ba108b?urlpath=%2Fopenrefine">
   <img src="https://mybinder.org/badge.svg" alt="Binder" class="figure mx-auto d-block">
</figure>

This is a problem in the fix_figures() function where we assume that the parent element of the image is a paragraph. This needs to change the code on line 58 to explicitly search for the first ancestor that is a paragraph element for this image (though this may fail for inline images)

sandpaper/R/utils-xml.R

Lines 54 to 68 in 3d088c7

fix_figures <- function(nodes = NULL) {
if (length(nodes) == 0) return(nodes)
figs <- xml2::xml_find_all(nodes, ".//img")
caps <- xml2::xml_find_all(nodes, ".//p[@class='caption']")
fig_element <- xml2::xml_parent(figs)
classes <- xml2::xml_attr(figs, "class")
classes <- ifelse(is.na(classes), "", classes)
classes <- paste(classes, "figure mx-auto d-block")
xml2::xml_set_attr(figs, "class", trimws(classes))
xml2::xml_set_name(caps, "figcaption")
xml2::xml_set_attr(caps, "class", NULL)
xml2::xml_set_name(fig_element, "figure")
xml2::xml_set_attr(fig_element, "class", NULL)
invisible(nodes)
}

@zkamvar zkamvar added bug Something isn't working frequency: low indicator that a use-case has a low-frequency in lessons labels Apr 26, 2023
@zkamvar zkamvar self-assigned this Apr 26, 2023
zkamvar added a commit that referenced this issue Apr 27, 2023
@zkamvar zkamvar changed the title markdown linked images have the href attribute added to the figure element inline figures should not be converted to figures Apr 27, 2023
zkamvar added a commit that referenced this issue Apr 28, 2023
@zkamvar
Copy link
Contributor Author

zkamvar commented Apr 28, 2023

The solution was surprisingly straightforward:

in: 1b6af38:

-figs <- xml2::xml_find_all(nodes, ".//img")
+figs <- xml2::xml_find_all(nodes, ".//div[@class='figure']/img")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working frequency: low indicator that a use-case has a low-frequency in lessons
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant