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

Anchor tags should not be used as buttons #110

Conversation

Matrix278
Copy link
Contributor

@Matrix278 Matrix278 commented Oct 2, 2021

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Screenshot(s)

image

Description

Resolves: #104

Checklist:

  • I have thoroughly read the CONTRIBUTING guidelines.
  • I understand my pull request will be thoroughly reviewed at high detail.
  • I understand the work in my pull request will only be available in the next version release of Checka11y.css and not in the current version release.
  • I confirm the work in this pull request is valid according to my findings and is not something for anything personal.
  • I have updated the README and/or features.md where and if applicable (still put an x if you have considered this but thought there was nothing to add or modify).
  • I have added myself to the contributors section in package.json (still put an x if you have considered this but decided not to add yourself).
  • I have checked I have not committed any accidental files.
  • I have tested all the main modern browsers (I.e. Chrome, Firefox, Edge, Safari - please leave this unchecked if there were any browsers listed you could not test and list them in the help section with details why you couldn't test that browser)
  • I have run the automated tests and added new ones to cover new code.
  • All new and existing a11y checks still work correctly (compare your local test/index.html to the test/index.html in the master branch).

Help

Could not test in Safari because I do not have access to an Apple device

There is one issue with an existing feature E0009, where the invalid HTML elements are nested inside a button. As this new feature is checking for empty href="#", it will trigger a new message here as well. How I could change this? Does here should be added check if is not a child of a button or it could be done another way? Need some help with that, what I could do about this?

<button>
        <a href="#">Hello, I am a link inside a &lt;button&gt;</a>
</button>

image

checka11y-warnings.css Outdated Show resolved Hide resolved
@jackdomleo7 jackdomleo7 self-requested a review October 3, 2021 13:11
@jackdomleo7 jackdomleo7 added a11y feature New feature or request for an a11y check Hacktoberfest Hacktoberfest eligible labels Oct 3, 2021
@jackdomleo7
Copy link
Owner

Hi @Matrix278, thank you for this PR! That's an interesting thing you have found. Are you saying the warning W0010 overrides the error if both apply? 🤔

@Matrix278
Copy link
Contributor Author

Matrix278 commented Oct 3, 2021

Hi @Matrix278, thank you for this PR! That's an interesting thing you have found. Are you saying the warning W0010 overrides the error if both apply? 🤔

Hi, yes it seems like it overrides it and showing the new warning there now, where actually should be error. I guess some additional css should be added?

Copy link
Owner

@jackdomleo7 jackdomleo7 left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. Really good work. Just a few minor changes then we'd be good to go 🙂

codes.md Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/warnings/features/_links.scss Outdated Show resolved Hide resolved
@jackdomleo7
Copy link
Owner

That's interesting. I don't think any additional CSS is needed. After your PR is merged, I'll do some investigation before I publish a new version because I think I have an idea what it may be.


FYI - You can ignore the linting check failure, there is an open issue (#87) where I'm struggling to resolve this.

@Matrix278
Copy link
Contributor Author

That's interesting. I don't think any additional CSS is needed. After your PR is merged, I'll do some investigation before I publish a new version because I think I have an idea what it may be.

FYI - You can ignore the linting check failure, there is an open issue (#87) where I'm struggling to resolve this.

Sure, thanks for clarifying 👍

@jackdomleo7
Copy link
Owner

Hi @Matrix278, we have recently merged #111. Would you be able to update your branch with master and where you have added your new feature as W0010, could you change it to W0011 as W0010 now already exists due to the recent merge 🙂

@Matrix278
Copy link
Contributor Author

Matrix278 commented Oct 3, 2021

@jackdomleo7 ready for review again, fixed all your suggestions and conflicts, should be fine now

Copy link
Owner

@jackdomleo7 jackdomleo7 left a comment

Choose a reason for hiding this comment

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

Nice one @Matrix278! Thank you.

@jackdomleo7 jackdomleo7 merged commit de8db98 into jackdomleo7:master Oct 3, 2021
@Matrix278 Matrix278 deleted the anchor-tags-should-not-be-used-as-buttons branch October 3, 2021 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y feature New feature or request for an a11y check Hacktoberfest Hacktoberfest eligible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Anchor tags should not be used as buttons
2 participants