[Fix] no-unknown-property: support onBeforeToggle, popoverTarget, popoverTargetAction attributes
#3865
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3865 +/- ##
==========================================
+ Coverage 97.70% 97.80% +0.09%
==========================================
Files 136 136
Lines 9950 9978 +28
Branches 3685 3703 +18
==========================================
+ Hits 9722 9759 +37
+ Misses 228 219 -9 ☔ View full report in Codecov by Sentry. |
… `popoverTargetAction` attributes
acusti
force-pushed
the
popover-part-2
branch
from
73a1461 to
4a5678d
Compare
| <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' }, | ||
| }, |
There was a problem hiding this comment.
it seems like this test passed previously - instead of changing the existing one, can we add a new test?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
if it already supported the all-lowercase one, then yes, it'd be semver-major to stop supporting it.
Fixes #3864