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

[wip] Check images for alt text #1800

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ jobs:

- name: File metadata
run: npm run check:metadata
- name: Check image alt text
run: npm run check:alt-text
- name: Spellcheck
run: npm run check:spelling
- name: Check Qiskit bot config
Expand Down
14 changes: 14 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit, "alt text" isn't code so no need to escape it.

Suggested change
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Only check file metadata
# Only check alt text

npm run check:alt-text

# Or, run all the checks. Although this only checks non-API docs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Or, run all the checks. Although this only checks non-API docs.
# Or, run all the checks

npm run check
```

## Spellcheck

We use [cSpell](https://cspell.org) to check for spelling. The `lint` job in CI will fail if there are spelling issues.
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@
"fmt": "prettier --write .",
"test": "playwright test",
"typecheck": "tsc",
"check": "npm run check:qiskit-bot && npm run check:patterns-index && npm run check:metadata && npm run check:spelling && npm run check:internal-links && npm run check:orphan-pages && npm run check:fmt",
"check": "npm run check:qiskit-bot && npm run check:patterns-index && npm run check:alt-text && npm run check:metadata && npm run check:spelling && npm run check:internal-links && npm run check:orphan-pages && npm run check:fmt",
"check:metadata": "tsx scripts/js/commands/checkMetadata.ts",
"check:alt-text:": "tsx scripts/js/commands/checkAltText.ts",
"check:spelling": "tsx scripts/js/commands/checkSpelling.ts",
"check:fmt": "prettier --check .",
"check:internal-links": "tsx scripts/js/commands/checkInternalLinks.ts",
Expand Down
97 changes: 97 additions & 0 deletions scripts/js/commands/checkAltText.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
// This code is a Qiskit project.
//
// (C) Copyright IBM 2024.
//
// This code is licensed under the Apache License, Version 2.0. You may
// obtain a copy of this license in the LICENSE file in the root directory
// of this source tree or at http://www.apache.org/licenses/LICENSE-2.0.
//
// Any modifications or derivative works of this code must retain this
// copyright notice, and modified files need to carry a notice indicating
// that they have been altered from the originals.

import { globby } from "globby";
import yargs from "yargs/yargs";
import { hideBin } from "yargs/helpers";

import { extractAlt } from "../lib/links/extractAltText.js";
import { readMarkdown } from "../lib/markdownReader.js";

interface Arguments {
[x: string]: unknown;
}

const readArgs = (): Arguments => {
return yargs(hideBin(process.argv))
.version(false)
.parseSync();
};
Comment on lines +20 to +28
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for this. This program doesn't need to take arguments


async function determineFiles(args: Arguments): Promise<[string[], string[]]> {
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)];
Comment on lines +31 to +33
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}


function getNullAlt(
Copy link
Collaborator

Choose a reason for hiding this comment

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

You won't need this function if you take my recommendation in extractAltText.ts

alt: Set<string | null | undefined>,
imgPath: Set<string>,
Err: string[]
): void {
const altArray = Array.from(alt);
const imgArray = Array.from(imgPath);

altArray.forEach((alt, i) => {
if(alt===null) {
Err.push(imgArray[i]);
}
});
}


const main = async (): Promise<void> => {
const args = readArgs();
const [mdFiles, notebookFiles] = await determineFiles(args);

const mdErrors: string[] = [];
for (const file of mdFiles) {
const markdown = await readMarkdown(file);
const{ imageName, altText } = await extractAlt(markdown);
getNullAlt(altText, imageName, mdErrors);
}

const notebookErrors: string[] = [];
for (const file of notebookFiles) {
const markdown = await readMarkdown(file);
const{ imageName, altText } = await extractAlt(markdown);
getNullAlt(altText, imageName, notebookErrors);

}
handleErrors(mdErrors, notebookErrors);
};

function handleErrors(mdErrors: string[], notebookErrors: string[]): void {
if (mdErrors.length > 0) {
console.error(`
Image does not have alt text. Every image must have associated alt text like this:
![Alt text goes here](/random-img.png)

Please fix these images:\n\n${mdErrors.join("\n")}
`)
}
if (notebookErrors.length > 0) {
console.error(`
Image does not have alt text. Every image in markdown must have associated alt text like this:
![Alt text goes here](/random-img.png)

Please fix these images:\n\n${notebookErrors.join("\n")}
`);
}
if (mdErrors.length > 0 || notebookErrors.length > 0) {
process.exit(1);
}
}

main();

36 changes: 36 additions & 0 deletions scripts/js/lib/links/extractAltText.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// This code is a Qiskit project.
//
// (C) Copyright IBM 2024.
//
// This code is licensed under the Apache License, Version 2.0. You may
// obtain a copy of this license in the LICENSE file in the root directory
// of this source tree or at http://www.apache.org/licenses/LICENSE-2.0.
//
// Any modifications or derivative works of this code must retain this
// 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";

Check failure on line 13 in scripts/js/lib/links/extractAltText.test.ts

View workflow job for this annotation

GitHub Actions / Lint

Cannot find module '@jest/globals' or its corresponding type declarations.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't use Jest anymore. We've now migrated to Playwright. You can look at some other test files for inspiration.

import { extractAlt, AltText } from "./extractAltText";

test("extractAlt()", async () => {
const markdown = `
# A header
![Our first image with alt text](/images/img1.png)

![](/images/invalid_img1.png)

![Here's another valid image](/images/img3.png)

![](/images/invalid_img2.png)

![And now, our last link](https://ibm.com)
`;
const{ imageName, altText } = await extractAlt(markdown);
expect(imageName).toEqual(
new Set(["/images/img1.png", "/images/invalid_img1.png", "/images/img3.png", "/images/invalid_img2.png", "https://ibm.com"]),
);
expect(altText).toEqual(
new Set(["Our first image with alt text", null, "Here's another valid image", null, "And now, our last link"]),
);
});
49 changes: 49 additions & 0 deletions scripts/js/lib/links/extractAltText.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// This code is a Qiskit project.
//
// (C) Copyright IBM 2024.
//
// This code is licensed under the Apache License, Version 2.0. You may
// obtain a copy of this license in the LICENSE file in the root directory
// of this source tree or at http://www.apache.org/licenses/LICENSE-2.0.
//
// Any modifications or derivative works of this code must retain this
// copyright notice, and modified files need to carry a notice indicating
// that they have been altered from the originals.

import { visit } from "unist-util-visit";
import { unified } from "unified";
import { Root } from "remark-mdx";
import remarkStringify from "remark-stringify";
import rehypeRemark from "rehype-remark";
import rehypeParse from "rehype-parse";
import remarkGfm from "remark-gfm";

export interface AltText {
imageName: Set<string>;
altText: Set<string | null | undefined>;
};


export async function extractAlt(
markdown: string,
): Promise<AltText> {
Comment on lines +21 to +29
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

const imageName = new Set<string>();
const altText = new Set<string | null | undefined>();

const getAlt = (link: string, a: string | null | undefined): void => {
imageName.add(link);
altText.add(a);
};

await unified()
.use(rehypeParse)
.use(remarkGfm)
.use(rehypeRemark)
.use(() => (tree: Root) => {
visit(tree, "image", (TreeNode) => getAlt(TreeNode.url, TreeNode.alt));
})
.use(remarkStringify)
.process(markdown);

return { imageName, altText };
}
Loading