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

[New] label-has-associated-control: Add glob support #749

Conversation

hallzac2
Copy link
Contributor

@hallzac2 hallzac2 commented Oct 8, 2020

Description

Add glob support for label-has-associated-control rule. This will allow consumers of this library to have some more powerful matching capabilities when using the controlComponents option.

To accomplish this, I brought in the minimatch library => https://github.com/isaacs/minimatch

There are tons of other libraries to do this same thing. It shouldn't be too hard to switch to any of the others if preferred or a lightweight implementation could be done to support the bare minimum.

This addresses issue #720.

Testing

  1. Pull the branch
  2. Run npm i
  3. Run npm run test
  4. Try out some different globs to ensure that this meeting the feature set that it should be

@hallzac2
Copy link
Contributor Author

hallzac2 commented Oct 8, 2020

It looks like the build is failing for older version of node 😦 I looked at micromatch's node engine and it lists "node": ">=8.6" so it won't work with this. I'll see if I can find a different solution.

@hallzac2
Copy link
Contributor Author

hallzac2 commented Oct 8, 2020

I switched to using minimatch since it should be compatible. The build is now passing! 😄

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Seems good overall, just a few comments.

@@ -101,7 +101,7 @@ This rule takes one optional object argument of type object:

`labelComponents` is a list of custom React Component names that should be checked for an associated control.
`labelAttributes` is a list of attributes to check on the label component and its children for a label. Use this if you have a custom component that uses a string passed on a prop to render an HTML `label`, for example.
`controlComponents` is a list of custom React Components names that will output an input element.
`controlComponents` is a list of custom React Components names that will output an input element. Glob format is also supported for names.
Copy link
Member

Choose a reason for hiding this comment

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

let's add some examples, or a link to what "glob format" means.

__tests__/src/util/mayContainChildComponent-test.js Outdated Show resolved Hide resolved
@ljharb ljharb changed the title Add glob support for label-has-associated-control rule [New] label-has-associated-control: Add glob support Oct 10, 2020
@jessebeach
Copy link
Collaborator

Seems really close but for a few nits.

@hallzac2 hallzac2 requested a review from ljharb November 2, 2020 15:51
@hallzac2
Copy link
Contributor Author

hallzac2 commented Nov 2, 2020

@ljharb I incorporated the requested changes and add documentation / examples for glob format. Sorry about the long turn around time on this, life got a little hectic the past few weeks.

@jessebeach
Copy link
Collaborator

@ljharb I incorporated the requested changes and add documentation / examples for glob format. Sorry about the long turn around time on this, life got a little hectic the past few weeks.

No need to apologize! This is open source; we're all volunteering our time. Thank you for circling back to this one!

@jessebeach jessebeach self-requested a review November 28, 2020 22:22
@jessebeach
Copy link
Collaborator

@ljharb good to go?

@ljharb ljharb force-pushed the zhall1-add-glob-support-to-label-has-associated-control branch from 64c6eb0 to 7d5511d Compare December 26, 2020 17:47
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Also sorry for the delay on my end :-)

@ljharb ljharb merged commit 7d5511d into jsx-eslint:master Dec 26, 2020
@danantal danantal mentioned this pull request Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants