Skip to content

Anchor tags should not be used as buttons#110

Merged
jackdomleo7 merged 15 commits into
jackdomleo7:masterfrom
Matrix278:anchor-tags-should-not-be-used-as-buttons
Oct 3, 2021
Merged

Anchor tags should not be used as buttons#110
jackdomleo7 merged 15 commits into
jackdomleo7:masterfrom
Matrix278:anchor-tags-should-not-be-used-as-buttons

Conversation

@Matrix278

@Matrix278 Matrix278 commented Oct 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Matrix278
Matrix278 commented Oct 2, 2021
View reviewed changes
Comment thread checka11y-warnings.css Outdated
@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

jackdomleo7 commented Oct 3, 2021

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

Matrix278 commented Oct 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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?

jackdomleo7
jackdomleo7 requested changes Oct 3, 2021
View reviewed changes

@jackdomleo7 jackdomleo7 left a comment

Copy link
Copy Markdown
Owner

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 🙂

Comment thread codes.md
Comment thread package.json Outdated
Comment thread src/warnings/features/_links.scss Outdated
@jackdomleo7

jackdomleo7 commented Oct 3, 2021

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

Matrix278 commented Oct 3, 2021

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

jackdomleo7 commented Oct 3, 2021

Copy link
Copy Markdown
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 Matrix278 requested a review from jackdomleo7 October 3, 2021 15:57
@Matrix278

Matrix278 commented Oct 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

jackdomleo7
jackdomleo7 approved these changes Oct 3, 2021
View reviewed changes

@jackdomleo7 jackdomleo7 left a comment

Copy link
Copy Markdown
Owner

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

Reviewers

@jackdomleo7 jackdomleo7 jackdomleo7 approved these changes

Assignees

@Matrix278 Matrix278

Labels

a11y feature New feature or request for an a11y check Hacktoberfest Hacktoberfest eligible

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Anchor tags should not be used as buttons

2 participants

@Matrix278 @jackdomleo7