-
Notifications
You must be signed in to change notification settings - Fork 791
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
feat: add EN.301.549 tags to rules #4063
Conversation
b543125
to
d6febf9
Compare
build/tasks/validate.js
Outdated
name: 'Section 508', | ||
standardRegex: /^section508$/, | ||
criterionRegex: /^section508\.\d{1,2}\.[a-z]$/, | ||
wcagLevelRegex: /wcag2aa?/ |
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.
Should the wcagLevelRegex
es be terminated by ^
and $
to avoid, say, matching wcag2aaa
by accident?
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.
Yes it should :) Fixed
build/tasks/validate.js
Outdated
}, | ||
{ | ||
name: 'EN 301 549', | ||
standardRegex: /^EN.301.549$/, |
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.
standardRegex: /^EN.301.549$/, | |
standardRegex: /^EN\.301\.549$/, |
]; | ||
|
||
const standardsTags = [ | ||
{ |
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.
It's non-obvious that findTagIssues
relies on the WCAG
entry being first in the list, maybe leave a comment noting that that's important?
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.
I think the tag validation should be its own PR and that we had some tests around the validation (might be hard since it's in a build step, but it's getting complex now that it's not strictly using the schema validator).
|
||
// Other tags | ||
const usedMiscTags = miscTags.filter(tag => tags.includes(tag)); | ||
if (!startsWith(tags, usedMiscTags)) { |
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.
The only thing left in the array at this point is misc tags or unknown tags, so stating order seems irrelevant as the unknown tags will be flagged for removal.
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 checks misc tags are in the correct order. The issue message can be better here though. I'll change that.
'time-and-media' | ||
]; | ||
|
||
const standardsTags = [ |
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.
Should we enforce that the order of tags follows this array? Right now it doesn't matter if section508 comes before EN-9, but does that matter?
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.
It does a little, yes. The order in which we list tags in the rule is the order in which they show up in other places. Having a consistent order will make it a little easier for people to find the tag they are looking for. It's minor, but it helps I think.
doc/API.md
Outdated
| `section508.*.*` | Requirement in old Section 508 | | ||
| `TTv5` | Trusted Tester v5 rules | | ||
| `TT*.*` | Test ID in Trusted Tester | | ||
| `EN.301.549` | Rule required under [EN 301 549](https://www.etsi.org/deliver/etsi_en/301500_301599/301549/03.02.01_60/en_301549v030201p.pdf) | |
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.
The .
separators here feel a bit inconsistent with other tags as a separator for what are spaces in the official standard being referenced. Other existing cases either omit a separator (Section 508
-> section508
) or use a hyphen (best-bractice
). I think EN-301-549
is the nicest-looking consistent option, but I think any of EN301549
, en301549
, en-301-549
would be fine.
This feedback is minor; I don't think creates big backcompat issues, so if anyone else felt strongly about the .
s, I can live with the inconsistency.
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.
Dashes make sense. @straker what do you think of EN-301-549
?
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.
I'm good with dashes
doc/API.md
Outdated
| `TTv5` | Trusted Tester v5 rules | | ||
| `TT*.*` | Test ID in Trusted Tester | | ||
| `EN.301.549` | Rule required under [EN 301 549](https://www.etsi.org/deliver/etsi_en/301500_301599/301549/03.02.01_60/en_301549v030201p.pdf) | | ||
| `EN-9.*` | Section in EN 301 549 listing the requirement | |
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.
Using just EN
without any other prefix makes me nervous about creating future backcompat issues; I think future revisions of EN 301 549 are likely to maintain consistent section numbering, but if there's ever a future different EN standard to, say, cover WCAG 3, it's reasonably likely it'd use conflicting numbering. That leads me to prefer using a fuller prefix here, like EN-301-549-9.1.2.3
or EN-301-549.9.1.2.3
.
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.
We have that problem with other tags too. wcag111
won't be relevant when WCAG 3 comes out. TT1.a
will probably be obsolete when Trusted Tester v6 comes out. Don't quite know what we're going to do at that point but it seems likely we need to come up with something different by that time.
I can well imagine we deprecate the criteria/section specific tags all together and put that information somewhere else. That's what we did with the ACT Rules, we added an actId
prop to the rule.
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.
As discussed, the plan is to eventually not include SC numbers / section numbers into tags but make them available in some other format in the output. For now we're going with what we've been doing up until this point.
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.
Thanks Wilco! I love the new validation, gave me much more confidence that my eyes didn't glaze over something during review. A few suggestions inline.
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.
LGTM!
Closes: #4051