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: enhance prefer-enabled-disabled rule to support the aria-disabled attr #5

Closed
wants to merge 1 commit into from

Conversation

gnapse
Copy link
Member

@gnapse gnapse commented Oct 31, 2019

In light of toBeEnabled and toBeDisabled being enhanced with support for the aria-disabled attribute (testing-library/jest-dom#146) I wanted to go ahead and extend the corresponding eslint rule to detect that attribute as well.

Note: This should not be merged yet, until that change in jest-dom is released, but I was so excited about this eslint plugin and also wanted to make my first contribution ever to a eslint plugin, that I went ahead and did it. I thought it'd take more time but this is very nicely done and was easy to jump in. Still, I'm not sure I'm doing it all right, so let me know.

@gnapse gnapse added the enhancement New feature or request label Oct 31, 2019
@gnapse gnapse requested a review from benmonro October 31, 2019 15:04
@gnapse gnapse self-assigned this Oct 31, 2019
@benmonro
Copy link
Member

sweet, LGTM. just let me know when I can merge

Copy link
Member

@benmonro benmonro left a comment

Choose a reason for hiding this comment

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

:shipit:

@benmonro benmonro changed the title Enhance prefer-enabled-disabled rule to support the aria-disabled attr feat: enhance prefer-enabled-disabled rule to support the aria-disabled attr Nov 4, 2019
@benmonro
Copy link
Member

@gnapse any update on this? I'd love to merge.

@gnapse
Copy link
Member Author

gnapse commented Nov 13, 2019

Thanks for the follow up, as I had forgotten about this.

I recommend closing this. You can check out the discussion in the linked issue in jest-dom that these two are really semantically very different (disabled vs aria-disabled) and it's not advisable to have the both covered under the same matcher.

Feel free to chime in over there if you have some take on all this, or otherwise I'd leave it to you to close this if you agree this should not go through.

@benmonro
Copy link
Member

ok closing. feel free to add a rule for toBeAriaDisabled() if that ever gets implemented, would love that.

@benmonro benmonro closed this Nov 13, 2019
@gnapse gnapse deleted the support-aria-disabled branch November 13, 2019 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request NO MERGE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants