[wip] Check images for alt text #1800
Conversation
|
@Eric-Arellano please review this when you're free, thanks! |
| @@ -284,6 +284,20 @@ npm run check:metadata -- --apis | |||
| npm run check | |||
| ``` | |||
|
|
|||
| ## Check image alt text | |||
|
|
|||
| Every image needs to have `alt text` for accessibility. The `lint` job in CI will fail if images do not have alt text. | |||
There was a problem hiding this comment.
Nit, "alt text" isn't code so no need to escape it.
| Every image needs to have `alt text` for accessibility. The `lint` job in CI will fail if images do not have alt text. | |
| Every image needs to have alt text for accessibility. The `lint` job in CI will fail if images do not have alt text. |
| You can also check for valid metadata locally: | ||
|
|
||
| ```bash | ||
| # Only check file metadata |
There was a problem hiding this comment.
| # Only check file metadata | |
| # Only check alt text |
| # Only check file metadata | ||
| npm run check:alt-text | ||
|
|
||
| # Or, run all the checks. Although this only checks non-API docs. |
There was a problem hiding this comment.
| # Or, run all the checks. Although this only checks non-API docs. | |
| # Or, run all the checks |
| interface Arguments { | ||
| [x: string]: unknown; | ||
| } | ||
|
|
||
| const readArgs = (): Arguments => { | ||
| return yargs(hideBin(process.argv)) | ||
| .version(false) | ||
| .parseSync(); | ||
| }; |
There was a problem hiding this comment.
No need for this. This program doesn't need to take arguments
| const mdGlobs = ["docs/guides/*.mdx", "docs/migration-guides/*.mdx"]; | ||
| const notebookGlobs = ["docs/guides/*.ipynb", "docs/migration-guides/*.ipynb"]; | ||
| return [await globby(mdGlobs), await globby(notebookGlobs)]; |
There was a problem hiding this comment.
No need to differentiate between MDX and ipynb. We do that for checkMetadata because you set up the metadata very differently there, but we don't do that for alt text.
Also, we want to check all folders other than docs/api. You can do something like:
return await globby(["docs/**/*.{ipynb,mdx}", "!docs/api/**/*.mdx"])
At that point, this function isn't really "pulling its weight". It would be cleaner to inline the code, meaning you get rid of the helper function and directly have const files = await globby(["docs/**/*.{ipynb,mdx}", "!docs/api/**/*.mdx"]) in your main function
| // copyright notice, and modified files need to carry a notice indicating | ||
| // that they have been altered from the originals. | ||
|
|
||
| import { expect, test } from "@jest/globals"; |
There was a problem hiding this comment.
We don't use Jest anymore. We've now migrated to Playwright. You can look at some other test files for inspiration.
| export interface AltText { | ||
| imageName: Set<string>; | ||
| altText: Set<string | null | undefined>; | ||
| }; | ||
|
|
||
|
|
||
| export async function extractAlt( | ||
| markdown: string, | ||
| ): Promise<AltText> { |
There was a problem hiding this comment.
You can simply this functionality by making it more focused. All we care about is finding which images are missing alt text. We don't need to keep track of which images do have alt text. Note that if you are missing alt text, then altText would be empty. So, all we need is to return a string[] for the image names we're missing. You could rename the function something like findImagesWithoutAltText
| } | ||
|
|
||
|
|
||
| function getNullAlt( |
There was a problem hiding this comment.
You won't need this function if you take my recommendation in extractAltText.ts
Co-authored-by: Eric Arellano <[email protected]> Co-authored-by: Shraddha Aangiras <[email protected]>
This pull request adds an image checker to ensure all images have an alt text defined, which is crucial for accessibility, and that we don't have any `<img>` HTML tag. The output of the check shows the file name that contains invalid images and the images' names. Ex: ``` Error in file 'docs/guides/custom-transpiler-pass.ipynb': - The image '/images/guides/custom-transpiler-pass/DAG.png' does not have alt text. Invalid images found 💔 See https://github.com/Qiskit/documentation#images for instructions. ``` The PR builds on the work done by @shraddha-aangiras on #1800. I have made some changes to simplify the code and incorporated @Eric-Arellano's feedback. Thank you both for the work you've done! Closes #1651 --------- Co-authored-by: Eric Arellano <[email protected]> Co-authored-by: Shraddha Aangiras <[email protected]>
Implemented to ensure all images have some alt text
Fixes #1651