Reuse and delete alt-text-bot comment to update lint issues. #55
Conversation
There was a problem hiding this comment.
Copilot reviewed 28 out of 28 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (4)
src/index.test.js:10
- The error message should be consistent. Change 'alternate' to 'alternative' to match the rest of the messages.
expect(result).toBe("- Images should have alternate text (alt text) at line 1");
src/index.test.js:10
- The error message should be consistent. Change 'alternate' to 'alternative' to match the rest of the messages.
expect(result).toBe("- Images should have alternate text (alt text) at line 1");
src/index.test.js:10
- The error message should be consistent. Change 'alternate' to 'alternative' to match the rest of the messages.
expect(result).toBe("- Images should have alternate text (alt text) at line 1");
src/index.test.js:12
- The error message should be consistent. Change 'alternate' to 'alternative' to match the rest of the messages.
expect(result).toBe("- Images should have alternate text (alt text) at line 1");
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
| @@ -2,134 +2,135 @@ import { validate } from "./validate.js"; | |||
|
|
|||
| test("no-alt-text: should return errors", async () => { | |||
| let result = await validate("![]()"); | |||
| expect(result[0]).toBe("Images should have alternate text (alt text)"); | |||
| console.log(result) | |||
| expect(result).toBe("- Images should have alternate text (alt text) at line 1"); | |||
There was a problem hiding this comment.
Nit: This is pre-existing so maybe we want to follow up separately...
I think alt text stands for alternative text, and not alternate text!
There was a problem hiding this comment.
This is actually text that comes from the linter!
| pull_request: | ||
| types: [opened, edited] | ||
| issue_comment: | ||
| types: [created, edited] | ||
| types: [created, edited, deleted] | ||
| discussion: | ||
| types: [created, edited] | ||
| discussion_comment: | ||
| types: [created, edited] | ||
| types: [created, edited, deleted] |
There was a problem hiding this comment.
Is this in place so that the alt text comment by the bot can be deleted if the relevant comment is deleted?
EDIT: ooh yeah looks like it!
There was a problem hiding this comment.
If a consumer upgrades to the new version but forgets to add this, will the action start failing, or will it fail silently?
If it's a breaking change, I wonder if we'd want to bump to major version with a note 🤔.
Let's also update the config in the README!
There was a problem hiding this comment.
It shouldn't fail! They just won't get their comments deleted. (I tested this!) AND GOOD CALL i'll add this to the README.
Closes:
What
Reuse and delete messages
This PR updates the accessibility-alt-text-bot to reuse an existing accessibility-alt-text-bot comment when it corresponds to the same content that needs updating.
For example, if alt text is missing in a PR description and a user adds another inaccessible image to the description then the accessibility-alt-text-bot comment pertaining to that description will be updated. However, if a new author adds an inaccessible image to a new comment, then a new accessibility-alt-text-bot comment will be created.
Additionally, the accessibility-alt-text-bot will delete its own comment once the linting errors have been resolved.
Add clarity to message
I also updated our alt-text-bot message to give more context on where the error is occurring: