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

wip: move a11y to its on file #3725

Closed
wants to merge 6 commits into from

Conversation

tanhauhau
Copy link
Member

@tanhauhau tanhauhau commented Oct 17, 2019

Hi @Rich-Harris i was looking at the issue #820, and I was trying to look for the validate/a11y, and realise you've refactored the a11y validation into nodes/Element.ts in #1721.

I was thinking if I to add more a11y the Element.ts file would be very long, so, I tried to refactor out into a separate file. Would like to know your thoughts about this, before I continue to add more a11y validation.

Thanks


  • accessible-emoji
  • aria-activedescendant-has-tabindex
  • aria-proptypes
  • click-events-have-key-events
  • html-has-lang
  • img-redundant-alt
  • interactive-supports-focus
  • label-has-for
  • media-has-caption
  • mouse-events-have-key-events
  • no-interactive-element-to-noninteractive-role
  • no-noninteractive-element-interactions
  • no-noninteractive-element-to-interactive-role
  • no-noninteractive-tabindex
  • no-onchange
  • no-redundant-roles
  • no-static-element-interactions
  • role-has-required-aria-props
  • role-supports-aria-props

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests tests with npm test or yarn test)

@Rich-Harris
Copy link
Member

Thanks — I think this is a good move, yeah. Relatedly, I was thinking we should eventually colocate all the warnings, so that you could do something like this...

component.warn(`a11y-aria-attributes`, attribute, {
  name: this.name
});

...instead of this (the current code):

component.warn(attribute, {
  code: `a11y-aria-attributes`,
  message: `A11y: <${this.name}> should not have aria-* attributes`
});

That would make it much easier to build up some documentation about the various error/warning codes, some of which are slightly inscrutable at present. Is a separate issue though, I just mention it because it's tangentially related.

One nit in this PR: we use snake_case for internal variable names and methods, e.g. noMisplacedScope would be no_misplaced_scope.

@tanhauhau tanhauhau mentioned this pull request Oct 19, 2019
@Conduitry Conduitry marked this pull request as draft April 29, 2020 21:20
@Conduitry
Copy link
Member

I'm not against this change, but this PR is so old at this point that it would likely need a lot of work, and I'm trying to chip away at the open items slightly. Closing.

@Conduitry Conduitry closed this Mar 24, 2021
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

3 participants