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] add checked-requires-onchange-or-readonly rule #3680

Conversation

jaesoekjjang
Copy link
Contributor

@jaesoekjjang jaesoekjjang commented Jan 20, 2024

Resolved #3143

Hi, I added a new rule to enforce using onChange, readOnly props with checked.
How is it? And, any better naming for this rule?

Copy link

codecov bot commented Jan 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4edc1f0) 97.74% compared to head (3dfaaeb) 97.75%.

❗ Current head 3dfaaeb differs from pull request most recent head b9292b4. Consider uploading reports for the commit b9292b4 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3680   +/-   ##
=======================================
  Coverage   97.74%   97.75%           
=======================================
  Files         132      133    +1     
  Lines        9420     9458   +38     
  Branches     3456     3464    +8     
=======================================
+ Hits         9208     9246   +38     
  Misses        212      212           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ljharb
Copy link
Member

ljharb commented Jan 20, 2024

Can you elaborate on why?

@jaesoekjjang
Copy link
Contributor Author

jaesoekjjang commented Jan 20, 2024

Can you elaborate on why?

Sorry I'm no good at English. Are you asking about why this rule is necessary?

If it is, the answer is that there have been some times, using checked without onChange, and forgot to include readOnly like the code below

<tr onClick={toggleChecked}>
  <td>
     <input type='checkbox' checked={checked}/> 
  </td>
</tr>

Since there were no warns in the editor, I found bunch of red messages in the console later.
I guess the issue creator was also necessary for the same reason

@ljharb
Copy link
Member

ljharb commented Jan 20, 2024

Yes - but why would a checkbox always need to have an onChange or readOnly? If that's not a valid use case, why doesn't React warn on it? What's wrong with a checkbox that doesn't have any behavior - perhaps it's part of a larger form that gets submitted normally.

@jaesoekjjang
Copy link
Contributor Author

jaesoekjjang commented Jan 21, 2024

Checkbox doesn't need to have one of them always. The rule is checkingchecked property.
React warns on it in a console like this.
image
And this is the reason I thought it is necessary. To be warned earlier in editor.

I'm confusing, didnt' you labled accpeted, helpwanted, new rule on the issue #3143

@jaesoekjjang
Copy link
Contributor Author

So... do you think is it good to go or no? I can close the pr if it's not

@ljharb
Copy link
Member

ljharb commented Jan 31, 2024

Thanks for the reminder, that was 3 years ago :-) Let me give it a rereview.

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.

The only comment that's a blocker is the coverage one, i can clean up the rest if you don't feel like it :-)

docs/rules/checked-requires-onchange-or-readonly.md Outdated Show resolved Hide resolved
docs/rules/checked-requires-onchange-or-readonly.md Outdated Show resolved Hide resolved
docs/rules/checked-requires-onchange-or-readonly.md Outdated Show resolved Hide resolved
docs/rules/checked-requires-onchange-or-readonly.md Outdated Show resolved Hide resolved
lib/rules/checked-requires-onchange-or-readonly.js Outdated Show resolved Hide resolved
tests/lib/rules/checked-requires-onchange-or-readonly.js Outdated Show resolved Hide resolved
tests/lib/rules/checked-requires-onchange-or-readonly.js Outdated Show resolved Hide resolved
tests/lib/rules/checked-requires-onchange-or-readonly.js Outdated Show resolved Hide resolved
tests/lib/rules/checked-requires-onchange-or-readonly.js Outdated Show resolved Hide resolved
lib/rules/checked-requires-onchange-or-readonly.js Outdated Show resolved Hide resolved
@jaesoekjjang
Copy link
Contributor Author

jaesoekjjang commented Feb 1, 2024

The only comment that's a blocker is the coverage one, i can clean up the rest if you don't feel like it :-)

Sure, appreciate it 😊

@jaesoekjjang jaesoekjjang requested a review from ljharb February 1, 2024 12:00
@ljharb ljharb force-pushed the feat/checked-requires-onchange-or-readonly branch from 3dfaaeb to 381c7c1 Compare February 18, 2024 22:45
@ljharb ljharb force-pushed the feat/checked-requires-onchange-or-readonly branch from 381c7c1 to b70a926 Compare February 18, 2024 22:51
@ljharb ljharb changed the title [New Rule] checked-requires-onchange-or-readonly [New] add checked-requires-onchange-or-readonly rule Feb 18, 2024
@ljharb ljharb force-pushed the feat/checked-requires-onchange-or-readonly branch from b70a926 to b9292b4 Compare February 19, 2024 06:17
@ljharb ljharb merged commit b9292b4 into jsx-eslint:master Feb 19, 2024
309 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

New Rule: onChange or readOnly prop when checked is set on an input element
2 participants