-
Notifications
You must be signed in to change notification settings - Fork 1
Automate accessibility support section processing based on rule's input aspects #37
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
base: main
Are you sure you want to change the base?
Conversation
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.
Let me know if I misunderstand something, this is my first act-rules PR review :)
if (aspect.toLowerCase() in supportKeys) { | ||
body = updateAccessibilitySupportInBody( | ||
body, | ||
supportKeys[aspect.toLowerCase()] |
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.
you are converting aspect.toLowerCase()
twice, you can just do it once before the if statement
return body; | ||
} | ||
|
||
function updateAccessibilitySupportInBody( |
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.
Is there a chance some other parts of the body will need to be updated? If so, this should be abstracted to 1. Get that part by heading name (e.g. Accessibility Support in this case) 2. Update the part based on the string we need to add
body: string, | ||
support: string | ||
): string { | ||
const supportIndex = body.search(/\n###\s+Accessibility\s+Support\b/m); |
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.
Is it enforced somewhere that if a rule has accessibility support it will be exactly named "Accessibility Support"? Just want to make sure this can't be easily renamed and break this automation, or someone making a typo, e.g. "Acessibility Support"
I think anyway "Accessibility Support" and all other hardcoded strings, e.g. "There are no accessibility support issues known." should be constants, something along the lines
const SECTIONS = {
ACCESSIBILITY_SUPPORT: {
HEADING_PATTERN: /\n###\s+Accessibility\s+Support\b/mi,
HEADING: "Accessibility Support"
NO_ISSUES_TEXT: "There are no accessibility support issues known."
}
};
### Accessibility Support | ||
|
||
There are no accessibility support issues known. |
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.
is it always going to be accurately formatted, e.g. with line breaks?
// no support issues, replace with new support issue | ||
return ( | ||
body.substring(0, supportIndex) + | ||
`\n### Accessibility Support\n\n${support}\n\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.
is it a level 3 heading? I am seeing it as 2 here https://www.w3.org/WAI/standards-guidelines/act/rules/73f2c2/ but let me know if I am looking at the wrong page
@carlosapaduarte Can you also add a PR description (explain how / where it's going to update the accessibility support section), it will help me and any new people understand what this is doing better. Thank you! |
No description provided.