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

Add rule for at least one call to a11y helper #9

Merged
merged 1 commit into from
Jul 28, 2020

Conversation

backspace
Copy link
Collaborator

@backspace backspace commented Jul 27, 2020

Hey all, thanks for the work on this, nice to see 😍

I was looking into making a custom ESLint rule as a followup to adding some preliminary accessibility auditing so we can ensure each acceptance test file includes at least one audit call and I found your relevant plugin. As it exists, it has some overlap with what I was looking for, but it differs in that we aren’t looking to audit after every page interaction (for now, at least), and a11y-audit-after-test-helper wouldn’t quite work anyway because we mostly use page objects in our acceptance tests, which have highly customisable helper names.

There’s an undocumented a11y-audit rule that looks like an attempt to do something closer to what we would need, but it doesn’t support the settings override that would let me specify the custom audit helper wrapper I added to let us centralise our rule overrides, which is imported from nomad-ui/tests/helpers/a11y-audit instead of the Ember addon. Also, forthcoming in my accessibility work is adding auditing for integration/component tests, which the existing a11y-audit rule wouldn’t apply to because of its filename-checking. If you like, I could open another PR to delete the a11y-audit, as it seems to have been superseded 🤔

So this is a prototype new rule that looks for the audit helper import (with name/package override options). I didn’t include autofix because it would have to arbitrarily pick somewhere in the file to make the call, it seems preferable to let the developer insert it where it makes sense. I’ve tested it successfully locally with --rulesdir [local fork rules directory] and this addition to tests/.eslintrc.js:

  overrides: {
    files: ['acceptance/**/*-test.js'],
    rules: {
      'a11y-audit-called': 'error',
    },
    'settings': {
      'ember-a11y-testing': {
        'auditModule': {
          'package': 'nomad-ui/tests/helpers/a11y-audit',
          'exportName': 'default'
        }
      }
    }

This isn’t necessarily a complete PR, it might be that some more test cases would be helpful, for instance, though I didn’t exercise all the import name/package overrides as that seemed well-covered elsewhere. Also, I didn’t love having the a11yAuditImported failure, which seemed to overlap too much with the no-globals rule, but I was near the end of my timebox for this attempt, but if you think it’s inappropriate, I can try to rework things so it’s not needed.

One uncertainty is that I put the organisation name as a11y-tool-sandbox in the rule file’s url field despite the others using jgwhite; I gather the package was moved from originally belonging to @jgwhite and I see that redirects are working, so for more consistency I could use that as well.

Let me know if you think this is something you’d be into including in your plugin and whether there are any changes you’d like to see to make that possible? It would be nice to have it live alongside the existing rules and share base utilities, but if you don’t think it belongs, I can also adapt the helpful utilities and have it instead as an in-repository plugin for Nomad.

@jgwhite
Copy link
Collaborator

jgwhite commented Jul 28, 2020

Howdy @backspace 👋

I am so happy you found this project!!!

This PR looks great, thanks so much! ❤️

@fivetanley has been the driving force behind this library since I did the initial spike. Our hope is to eventually contribute it to https://github.com/ember-a11y. Having your contribution gives me confidence that the project will be widely useful.

@jgwhite jgwhite merged commit ed50362 into a11y-tool-sandbox:master Jul 28, 2020
@backspace
Copy link
Collaborator Author

Thank you, I was happy to find it, AST manipulation tends to be daunting for me but having something to build off of was quite helpful 🤩

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.

None yet

2 participants