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

feat: add EN.301.549 tags to rules #4063

Merged
merged 6 commits into from
Jun 29, 2023
Merged

feat: add EN.301.549 tags to rules #4063

merged 6 commits into from
Jun 29, 2023

Conversation

WilcoFiers
Copy link
Contributor

@WilcoFiers WilcoFiers commented Jun 23, 2023

  • Update tags
  • Update docs
  • Add tag validation

Closes: #4051

  • Make new tags available for DQU documentation (after merging)

@WilcoFiers WilcoFiers requested a review from a team as a code owner June 23, 2023 14:42
name: 'Section 508',
standardRegex: /^section508$/,
criterionRegex: /^section508\.\d{1,2}\.[a-z]$/,
wcagLevelRegex: /wcag2aa?/
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the wcagLevelRegexes be terminated by ^ and $ to avoid, say, matching wcag2aaa by accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it should :) Fixed

},
{
name: 'EN 301 549',
standardRegex: /^EN.301.549$/,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
standardRegex: /^EN.301.549$/,
standardRegex: /^EN\.301\.549$/,

];

const standardsTags = [
{
Copy link
Contributor

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?

Copy link
Contributor

@straker straker left a 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).

build/tasks/validate.js Outdated Show resolved Hide resolved
build/tasks/validate.js Outdated Show resolved Hide resolved
build/tasks/validate.js Outdated Show resolved Hide resolved
build/tasks/validate.js Show resolved Hide resolved

// Other tags
const usedMiscTags = miscTags.filter(tag => tags.includes(tag));
if (!startsWith(tags, usedMiscTags)) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

build/tasks/validate.js Outdated Show resolved Hide resolved
build/tasks/validate.js Show resolved Hide resolved
build/tasks/validate.js Show resolved Hide resolved
build/tasks/validate.js Outdated Show resolved Hide resolved
'time-and-media'
];

const standardsTags = [
Copy link
Contributor

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?

Copy link
Contributor Author

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) |
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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 |
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@dbjorge dbjorge left a 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.

@WilcoFiers WilcoFiers dismissed stale reviews from dbjorge and straker June 29, 2023 12:19

Updated

Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add EN301.549 tags
3 participants