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.

Sign up for GitHub

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

[Fix] no-unknown-property: support onBeforeToggle, popoverTarget, popoverTargetAction attributes #3865

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+12 −5
Draft

[Fix] no-unknown-property: support onBeforeToggle, popoverTarget, popoverTargetAction attributes #3865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange
* [Fix] types: correct generated type declaration ([#3840][] @ocavue)
* [`no-unknown-property`]: support `precedence` prop in react 19 ([#3829][] @acusti)
* [`prop-types`]: props missing in validation when using generic types from a namespace import ([#3859][] @rbondoc96)
* [`no-unknown-property`]: support `onBeforeToggle`, `popoverTarget`, `popoverTargetAction` attributes ([#3865][] @acusti)

### Changed
* [Tests] [`jsx-no-script-url`]: Improve tests ([#3849][] @radu2147)
Expand All @@ -23,6 +24,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange
[#3840]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3840
[#3833]: https://github.com/jsx-eslint/eslint-plugin-react/issues/3833
[#3829]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3829
[#3865]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3865

## [7.37.2] - 2024.10.22

Expand Down
8 changes: 5 additions & 3 deletions lib/rules/no-unknown-property.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ const DOM_PROPERTY_NAMES_ONE_WORD = [
// Video specific
'controls',
// popovers
'popover', 'popovertarget', 'popovertargetaction',
'popover',
];

const DOM_PROPERTY_NAMES_TWO_WORDS = [
Expand All @@ -259,8 +259,8 @@ const DOM_PROPERTY_NAMES_TWO_WORDS = [
'onCompositionUpdate', 'onCut', 'onDoubleClick', 'onDrag', 'onDragEnd', 'onDragEnter', 'onDragExit', 'onDragLeave',
'onError', 'onFocus', 'onInput', 'onKeyDown', 'onKeyPress', 'onKeyUp', 'onLoad', 'onWheel', 'onDragOver',
'onDragStart', 'onDrop', 'onMouseDown', 'onMouseEnter', 'onMouseLeave', 'onMouseMove', 'onMouseOut', 'onMouseOver',
'onMouseUp', 'onPaste', 'onScroll', 'onSelect', 'onSubmit', 'onToggle', 'onTransitionEnd', 'radioGroup', 'readOnly', 'referrerPolicy',
'rowSpan', 'srcDoc', 'srcLang', 'srcSet', 'useMap', 'fetchPriority',
'onMouseUp', 'onPaste', 'onScroll', 'onSelect', 'onSubmit', 'onBeforeToggle', 'onToggle', 'onTransitionEnd', 'radioGroup',
'readOnly', 'referrerPolicy', 'rowSpan', 'srcDoc', 'srcLang', 'srcSet', 'useMap', 'fetchPriority',
// SVG attributes
// See https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute
'crossOrigin', 'accentHeight', 'alignmentBaseline', 'arabicForm', 'attributeName',
Expand Down Expand Up @@ -317,6 +317,8 @@ const DOM_PROPERTY_NAMES_TWO_WORDS = [
'onMouseMoveCapture', 'onMouseOutCapture', 'onMouseOverCapture', 'onMouseUpCapture',
// Video specific
'autoPictureInPicture', 'controlsList', 'disablePictureInPicture', 'disableRemotePlayback',
// popovers
'popoverTarget', 'popoverTargetAction',
];

const DOM_PROPERTIES_IGNORE_CASE = ['charset', 'allowFullScreen', 'webkitAllowFullScreen', 'mozAllowFullScreen', 'webkitDirectory'];
Expand Down
7 changes: 5 additions & 2 deletions tests/lib/rules/no-unknown-property.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,14 @@ ruleTester.run('no-unknown-property', rule, {
{
code: `
<div>
<button popovertarget="my-popover" popovertargetaction="toggle">Open Popover</button>
<button popoverTarget="my-popover" popoverTargetAction="toggle">Open Popover</button>
<div popover id="my-popover">Greetings, one and all!</div>
<div id="my-popover" onBeforeToggle={this.onBeforeToggle} popover>Greetings, one and all!</div>
</div>
`,
settings: {
react: { version: '19.0.0' },
},
Comment on lines -185 to +192
Copy link
Member

Choose a reason for hiding this comment

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

it seems like this test passed previously - instead of changing the existing one, can we add a new test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb yeah that’s fair. just to confirm, however, do you want the plugin to allow both the all-lowercase version and the camelCased version of those attributes? before React added proper support in facebook/react#27981, the only way to use the attributes without getting console warning from react was to use the all-lowercase version of the attribute names. but it seems like now that the support has shipped in latest stable, it would be better to ensure that people always use the officially supported versions of the attributes, which is why i removed the all-lowercase versions of them (meaning that my changes make the original version of the test no longer pass).

i’m happy to do whichever seems best to you. there’s probably a semver consideration here also. i could even pretty easily redo the implementation so that for react >= 19, we replace the popovertarget and popovertargetaction attributes with the camel-cased version and then i can have test cases for both.

Copy link
Member

Choose a reason for hiding this comment

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

if it already supported the all-lowercase one, then yes, it'd be semver-major to stop supporting it.

},
]),
invalid: parsers.all([
Expand Down
Loading