-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add check for images' alt text #2348
Conversation
Co-authored-by: Eric Arellano <[email protected]> Co-authored-by: Shraddha Aangiras <[email protected]>
await unified() | ||
.use(remarkParse) | ||
.use(remarkGfm) | ||
.use(() => (tree: Root) => { | ||
visit(tree, "image", (node) => { | ||
if (!node.alt) { | ||
images.add(node.url); | ||
} | ||
}); | ||
}) | ||
.use(remarkStringify) | ||
.process(markdown); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice that the plugin will not extract images from HTML tags. I have searched in the repo and we don't have any instance of it. HTML tags can be used for the writers if they want to resize the images in markdown:
<img src="example.jpg" alt="Example" width="200"/>
We could implement this by adding another visitor that uses Cheerio as follows:
visit(tree, "html", (node) => {
const $ = load(node.value);
const imgElement = $("img");
if (imgElement.length) {
images.add(imgElement.attr("src"));
}
});
I think it would be good to add it to make it more robust, but what do you think?
visit(tree, "image", (node) => { | ||
if (!node.alt) { | ||
images.add(node.url); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: In my opinion, the visitor is already very simple and readable, but I discovered that we can add more conditions to the typeNode
part to make it more concise. Equivalent example:
visit(
tree,
(node) => node.type === "image" && !node.alt,
(node) => images.add(node.url),
);
I don't think is useful right now, but we could use it in the future if the logic becomes more complex.
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@@ -83,7 +83,7 @@ | |||
"dag = circuit_to_dag(qc)\n", | |||
"dag_drawer(dag)\n", | |||
"```\n", | |||
"![](/images/guides/custom-transpiler-pass/DAG.png)\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This image was missing the alt text. Looking at the notebook (https://docs.quantum.ibm.com/guides/custom-transpiler-pass) I think the alt text of the previous image (the circuit inside the "Background: DAG representation" collapsible) refereed to this one, so I moved it here and added an alt text for the circuit
Superseded by #2349