Skip to content

Conversation

@anenadic
Copy link
Collaborator

Fixes #507.

@github-actions
Copy link

github-actions bot commented Jan 27, 2026

Thank you!

Thank you for your pull request 😃

🤖 This automated message can help you check the rendered files in your submission for clarity. If you have any questions, please feel free to open an issue in {sandpaper}.

If you have files that automatically render output (e.g. R Markdown), then you should check for the following:

  • 🎯 correct output
  • 🖼️ correct figures
  • ❓ new warnings
  • ‼️ new errors

Rendered Changes

🔍 Inspect the changes: https://github.com/carpentries-incubator/python-intermediate-development/compare/md-outputs..md-outputs-PR-513

The following changes were observed in the rendered markdown documents:

 00-setting-the-scene.md       | 26 ++++++++++----------------
 10-section1-intro.md          | 35 +++++++++++++++--------------------
 14-collaboration-using-git.md | 31 ++++++++++++-------------------
 20-section2-intro.md          | 34 ++++++++++++++--------------------
 30-section3-intro.md          | 37 +++++++++++++++++--------------------
 40-section4-intro.md          | 40 +++++++++++++---------------------------
 41-code-review.md             | 10 +++-------
 50-section5-intro.md          | 41 +++++++++++++++--------------------------
 md5sum.txt                    | 16 ++++++++--------
 9 files changed, 107 insertions(+), 163 deletions(-)
What does this mean?

If you have source files that require output and figures to be generated (e.g. R Markdown), then it is important to make sure the generated figures and output are reproducible.

This output provides a way for you to inspect the output in a diff-friendly manner so that it's easy to see the changes that occur due to new software versions or randomisation.

⏱️ Updated at 2026-01-27 13:23:58 +0000

Copy link
Collaborator

@matt-graham matt-graham left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this so quickly @anenadic! The changes here look good in general to me. Some comments / questions:

  • Do we also want to remove the corresponding .svg versions of the Mermaid diagrams from episodes/fig now they are no longer used?
  • Are the HTML comments before each Mermaid fenced code block with the alt="..." contents functional / do these add alt attributes to the outer HTML element of the Markdown that follows? If so is this documented somewhere - I couldn't see anything from a quick search in Workbench documentation but wasn't sure exactly what the right search terms to use were!
  • As it looks like the accDescr syntax in the Mermaid diagram sources already gets rendered to a <svg><desc> element, which appears to be the recommended way of adding descriptive text to SVG content, is there still value in having the same (or similar) text attached as an alt attribute? From a don't-repeat-yourself perspective it feels like having the text duplicated will lead to it potentially getting out of sync, and from an accessibility perspective having multiple descriptions attached at different levels of a HTML element might actually be poorer from a user experience perspective for screen reader users?
  • For the section overview diagrams, in the preview of the updated Markdown rendered on GitHub, the reflowing of the longer bulleted text corresponding to the details of each section ends up a bit odd, making this difficult to read. In the SVGs previously used it looks like the corresponding nodes ended up automatically scaling width so this was less of an issue. At the moment we are using plain text labels for the nodes but Mermaid does support Markdown syntax in labels by enclosing within double-quotation marks and backticks e.g. "`Markdown _formatted_ __text__ `". That might be worth employing here for the longer labels to make the bulleted text more readable.

@anenadic
Copy link
Collaborator Author

anenadic commented Jan 28, 2026

Thanks for following up on this so quickly @anenadic! The changes here look good in general to me. Some comments / questions:

  • Do we also want to remove the corresponding .svg versions of the Mermaid diagrams from episodes/fig now they are no longer used?

Could do.

  • Are the HTML comments before each Mermaid fenced code block with the alt="..." contents functional / do these add alt attributes to the outer HTML element of the Markdown that follows? If so is this documented somewhere - I couldn't see anything from a quick search in Workbench documentation but wasn't sure exactly what the right search terms to use were!

They were left there as the accDescr does not get translated into alt-text by Mermaid so I kept these. Also accDescr and alt-text are not the same thing (alt-text is more descriptive for use by screen readers and accDescr is just used as an image caption - but I tried to align the text between the two as much as possible). So not sure what we want to do with this.

  • As it looks like the accDescr syntax in the Mermaid diagram sources already gets rendered to a <svg><desc> element, which appears to be the recommended way of adding descriptive text to SVG content, is there still value in having the same (or similar) text attached as an alt attribute? From a don't-repeat-yourself perspective it feels like having the text duplicated will lead to it potentially getting out of sync, and from an accessibility perspective having multiple descriptions attached at different levels of a HTML element might actually be poorer from a user experience perspective for screen reader users?
    As long as is picked up by screen readers then alt-text is redundant and we can remove the alt-text I left in HTML comments. If it is not picked up by screen readers then I am not sure how to add alt-text and I'd still keep my coments in the case this gets resolved in future.

    I can look into this - just did a quick translation. I wanted better formatting but did not have the time to explore this - thanks for pointing this out.

    Thank you @matt-graham.

    So, the actions are:

    • decide on what to do with alt-text comments
    • do better formatting of text

@bielsnohr
Copy link
Collaborator

I'll weigh in since I said I would have a look at Mermaid accessibility.

They were left there as the accDescr does not get translated into alt-text by Mermaid so I kept these. Also accDescr and alt-text are not the same thing (alt-text is more descriptive for use by screen readers and accDescr is just used as an image caption - but I tried to align the text between the two as much as possible). So not sure what we want to do with this.
--- @anenadic

This isn't quite accurate. After a bit of a dig myself, there is a difference depending on how the SVG is included in the HTML. In our case, Mermaid inlines the SVG inside a <svg> container, meaning that the alt attribute is not available, and instead some combination of <title>, <desc>, <aria-describedby>, <aria-labelledby> is recommended by the a11y-collective website. So, I think Mermaid is actually do something very close to correct by filling these tags in automatically using accDescr. Moreover, I don't think we want to mess with the internals of Mermaid to try to coerce it to put the SVG inside an <img> container just so we can have alt-text. Therefore, I agree with @matt-graham that we should get rid of the HTML comments and only use accDescr for Mermaid diagrams. I don't think the comments will get picked up by screen readers anyway.

I'm going to go ahead and make the requisite changes. I will also have a go at better formatting of text while I am there.

@bielsnohr
Copy link
Collaborator

A further update: I didn't appreciate that the accDescr gets put into the caption below the figure in the rendered HTML. This is slightly strange behaviour and I have raised an issue to see if it should be changed: carpentries/varnish#189

Regardless, we should still get rid of the alt-text comments, and they should be put into accDescr because that is what other lesson examples are doing.

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.

Move diagrams to use Mermaid where appropriate

3 participants