Skip to content

Commit

Permalink
eslint: Enable react/jsx-key rule (#39709)
Browse files Browse the repository at this point in the history
This rule requires a `key` prop be specified on elements that it thinks
probably need one, namely elements inside arrays or returned from
iterators for `.map()` and the like.

In practice most of the cases fixed here are probably pointless, since
the arrays are never going to be sorted or modified at runtime. But it's
easier to add a key than to write a comment justifing ignoring it in
each case.
  • Loading branch information
anomiex authored Oct 10, 2024
1 parent 77a67a6 commit 391bb78
Show file tree
Hide file tree
Showing 27 changed files with 94 additions and 28 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: fixed
Comment: Add `key` to React components in some stories. Should be no change to functionality.


Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ export const VariantsAndProps = () => {
<Text size="body-extra-small">no props</Text>
</Col>
{ variants.map( variant => (
<Col sm={ 4 } md={ 2 } lg={ 3 }>
<Col key={ 'normal-' + variant } sm={ 4 } md={ 2 } lg={ 3 }>
<Button { ...ButtonPrimary.args } variant={ variant } />
</Col>
) ) }
Expand All @@ -322,7 +322,7 @@ export const VariantsAndProps = () => {
<Text size="body-extra-small">size: small</Text>
</Col>
{ variants.map( variant => (
<Col sm={ 4 } md={ 2 } lg={ 3 }>
<Col key={ 'small-' + variant } sm={ 4 } md={ 2 } lg={ 3 }>
<Button { ...ButtonPrimary.args } variant={ variant } size="small" />
</Col>
) ) }
Expand All @@ -331,7 +331,7 @@ export const VariantsAndProps = () => {
<Text size="body-extra-small">weight: regular</Text>
</Col>
{ variants.map( variant => (
<Col sm={ 4 } md={ 2 } lg={ 3 }>
<Col key={ 'regular-' + variant } sm={ 4 } md={ 2 } lg={ 3 }>
<Button { ...ButtonPrimary.args } variant={ variant } weight="regular" />
</Col>
) ) }
Expand All @@ -340,7 +340,7 @@ export const VariantsAndProps = () => {
<Text size="body-extra-small">icon (cloud)</Text>
</Col>
{ variants.map( variant => (
<Col sm={ 4 } md={ 2 } lg={ 3 }>
<Col key={ 'icon-' + variant } sm={ 4 } md={ 2 } lg={ 3 }>
<Button
{ ...ButtonPrimary.args }
variant={ variant }
Expand All @@ -353,7 +353,7 @@ export const VariantsAndProps = () => {
<Text size="body-extra-small">disabled</Text>
</Col>
{ variants.map( variant => (
<Col sm={ 4 } md={ 2 } lg={ 3 }>
<Col key={ 'disabled-' + variant } sm={ 4 } md={ 2 } lg={ 3 }>
<Button { ...ButtonPrimary.args } variant={ variant } disabled />
</Col>
) ) }
Expand All @@ -362,7 +362,7 @@ export const VariantsAndProps = () => {
<Text size="body-extra-small">isDestructive</Text>
</Col>
{ variants.map( variant => (
<Col sm={ 4 } md={ 2 } lg={ 3 }>
<Col key={ 'destructive-' + variant } sm={ 4 } md={ 2 } lg={ 3 }>
<Button { ...ButtonPrimary.args } variant={ variant } isDestructive />
</Col>
) ) }
Expand All @@ -371,7 +371,7 @@ export const VariantsAndProps = () => {
<Text size="body-extra-small">isExternalLink</Text>
</Col>
{ variants.map( variant => (
<Col sm={ 4 } md={ 2 } lg={ 3 }>
<Col key={ 'external-' + variant } sm={ 4 } md={ 2 } lg={ 3 }>
<Button { ...ButtonPrimary.args } variant={ variant } isExternalLink />
</Col>
) ) }
Expand All @@ -380,7 +380,7 @@ export const VariantsAndProps = () => {
<Text size="body-extra-small">isLoading</Text>
</Col>
{ variants.map( variant => (
<Col sm={ 4 } md={ 2 } lg={ 3 }>
<Col key={ 'loading-' + variant } sm={ 4 } md={ 2 } lg={ 3 }>
<Button { ...ButtonPrimary.args } variant={ variant } isLoading />
</Col>
) ) }
Expand All @@ -389,7 +389,7 @@ export const VariantsAndProps = () => {
<Text size="body-extra-small">fullWidth</Text>
</Col>
{ variants.map( variant => (
<Col sm={ 4 } md={ 2 } lg={ 3 }>
<Col key={ 'fullwidth-' + variant } sm={ 4 } md={ 2 } lg={ 3 }>
<Button { ...ButtonPrimary.args } variant={ variant } fullWidth />
</Col>
) ) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ const Section = ( { title, data, children = null } ) => (
<h1 className={ styles.title }>{ title }</h1>
<Container fluid>
{ Object.keys( data ).map( key => (
<Col lg={ 3 } className={ styles.box }>
<Col key={ key } lg={ 3 } className={ styles.box }>
<Container fluid horizontalGap={ 2 }>
<Col className={ styles.key }>{ key }</Col>
{ children && <Col className={ styles.example }>{ children( data[ key ] ) }</Col> }
Expand Down Expand Up @@ -103,7 +103,11 @@ Tokens.parameters = {
export const Typographies = args => (
<div className={ styles[ 'instances-wrapper' ] }>
{ Object.keys( typography ).map( key => (
<div className={ styles[ 'font-instance' ] } style={ { fontSize: typography[ key ] } }>
<div
key={ key }
className={ styles[ 'font-instance' ] }
style={ { fontSize: typography[ key ] } }
>
{ args?.[ 'Text Instance' ] || `${ key } (${ typography[ key ] } )` }

<ClipboardButton variant="tertiary" text={ key } className={ styles[ 'copy-button' ] }>
Expand All @@ -125,6 +129,7 @@ export const Colors = () => (
<div className={ styles[ 'instances-wrapper' ] }>
{ Object.keys( colors ).map( key => (
<div
key={ key }
className={ styles[ 'color-instance' ] }
style={ { backgroundColor: colors[ key ], color: getContrast( colors[ key ] ) } }
>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: fixed
Comment: Add `key` to DisconnectSurvey options. Should be no change to functionality.


Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ const DisconnectSurvey = props => {
return options.map( option => {
return (
<SurveyChoice
key={ option.id }
id={ option.id }
onClick={ setSelectedAnswer }
onKeyDown={ handleAnswerKeyDown }
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: fixed
Comment: Add `key` to Notice actions. Should be no change to functionality.


Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ export function Instagram( props ) {
<Notice
hideCloseButton
actions={ [
<ExternalLink href={ getRedirectUrl( 'jetpack-social-share-to-instagram' ) }>
<ExternalLink
key="learn-more"
href={ getRedirectUrl( 'jetpack-social-share-to-instagram' ) }
>
{ __( 'Learn more', 'jetpack' ) }
</ExternalLink>,
] }
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fixed

Add `key` in React example to make it more correct.
1 change: 1 addition & 0 deletions projects/js-packages/social-logos/src/react/example.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ function SocialLogosExample() {

const allSocialLogos = SocialLogoData.map( logo => (
<SocialLogoItemExample
key={ logo.name }
name={ logo.name }
iconSize={ iconSize }
showIconNames={ showIconNames }
Expand Down
5 changes: 5 additions & 0 deletions projects/packages/forms/changelog/add-eslint-react-jsx-key
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: fixed
Comment: Add `key` in JetpackFieldControls controls. Should be no change to functionality.


Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ const JetpackFieldControls = ( {

let fieldSettings = [
<ToggleControl
key="required"
label={ __( 'Field is required', 'jetpack-forms' ) }
className="jetpack-field-label__required"
checked={ required }
Expand All @@ -115,6 +116,7 @@ const JetpackFieldControls = ( {
/>,
! hidePlaceholder && (
<TextControl
key="placeholderField"
label={ __( 'Placeholder text', 'jetpack-forms' ) }
value={ placeholder || '' }
onChange={ value => setAttributes( { [ placeholderField ]: value } ) }
Expand All @@ -124,8 +126,9 @@ const JetpackFieldControls = ( {
) }
/>
),
<JetpackFieldWidth setAttributes={ setAttributes } width={ width } />,
<JetpackFieldWidth key="width" setAttributes={ setAttributes } width={ width } />,
<ToggleControl
key="shareFieldAttributes"
label={ __( 'Sync fields style', 'jetpack-forms' ) }
checked={ attributes.shareFieldAttributes }
onChange={ value => setAttributes( { shareFieldAttributes: value } ) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ const GlobalNotice = ( { message, title, options } ) => {
const [ isBiggerThanMedium ] = useBreakpointMatch( [ 'md' ], [ '>' ] );

const actionButtons = options.actions?.map( action => {
return <ActionButton customClass={ styles.cta } { ...action } />;
return (
<ActionButton key={ action.key || action.label } customClass={ styles.cta } { ...action } />
);
} );

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ const ProductDetailTable = ( {
}
actions={ [
<Button
key="get"
variant="secondary"
href={ `https://wordpress.org/plugins/${ pluginSlug }` }
isExternalLink
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ export default function () {
actions={
tierPlansEnabled
? [
<Button isPrimary onClick={ upgradeClickHandler }>
<Button key="upgrade" isPrimary onClick={ upgradeClickHandler }>
{ showRenewalNotice ? renewalNoticeCta : upgradeNoticeCta }
</Button>,
]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: fixed
Comment: Add `key` to some React `actions` props. Should be no change to functionality.


4 changes: 4 additions & 0 deletions projects/packages/search/changelog/add-eslint-react-jsx-key
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fixed

Add `key` to tag and cat lists in `SearchResultMinimal` to improve behavior if lists change at runtime.
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class SearchResultMinimal extends Component {
{ tags.length !== 0 && (
<ul className="jetpack-instant-search__search-result-minimal-tags">
{ tags.map( tag => (
<li className="jetpack-instant-search__search-result-minimal-tag">
<li key={ tag } className="jetpack-instant-search__search-result-minimal-tag">
<Gridicon icon="tag" size={ this.getIconSize() } />
<span className="jetpack-instant-search__search-result-minimal-tag-text">
{ tag }
Expand All @@ -67,7 +67,7 @@ class SearchResultMinimal extends Component {
{ cats.length !== 0 && (
<ul className="jetpack-instant-search__search-result-minimal-cats">
{ cats.map( cat => (
<li className="jetpack-instant-search__search-result-minimal-cat">
<li key={ cat } className="jetpack-instant-search__search-result-minimal-cat">
<Gridicon icon="folder" size={ this.getIconSize() } />
<span className="jetpack-instant-search__search-result-minimal-cat-text">
{ cat }
Expand Down
6 changes: 4 additions & 2 deletions projects/plugins/jetpack/_inc/client/at-a-glance/scan.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -307,8 +307,10 @@ class DashScan extends Component {
return (
<>
{ renderActiveCard( [
<h2 className="jp-dash-item__count is-alert">{ numberFormat( numberOfThreats ) }</h2>,
<p className="jp-dash-item__description">
<h2 key="header" className="jp-dash-item__count is-alert">
{ numberFormat( numberOfThreats ) }
</h2>,
<p key="description" className="jp-dash-item__description">
{ createInterpolateElement(
_n(
'Security threat found. <a>Click here</a> to fix them immediately.',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export class ConnectionBanner extends React.Component {
<Notice
title={ title }
hideCloseButton
actions={ [ <ActionButton { ...connectButtonProps } /> ] }
actions={ [ <ActionButton key="connect" { ...connectButtonProps } /> ] }
>
{ description }
</Notice>
Expand Down
5 changes: 5 additions & 0 deletions projects/plugins/jetpack/changelog/add-eslint-react-jsx-key
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: other
Comment: Add `key` to various React things. Should be no change to functionality.


Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,15 @@ export default {
<RichText.Content tagName="p" value={ chooseAmountText } />
<div className="donations__amounts donations__one-time-item">
{ oneTimeDonation.amounts.map( amount => (
<div className="donations__amount" data-amount={ amount }>
<div key={ amount } className="donations__amount" data-amount={ amount }>
{ formatCurrency( amount, currency ) }
</div>
) ) }
</div>
{ monthlyDonation.show && (
<div className="donations__amounts donations__monthly-item">
{ monthlyDonation.amounts.map( amount => (
<div className="donations__amount" data-amount={ amount }>
<div key={ amount } className="donations__amount" data-amount={ amount }>
{ formatCurrency( amount, currency ) }
</div>
) ) }
Expand All @@ -138,7 +138,7 @@ export default {
{ annualDonation.show && (
<div className="donations__amounts donations__annual-item">
{ annualDonation.amounts.map( amount => (
<div className="donations__amount" data-amount={ amount }>
<div key={ amount } className="donations__amount" data-amount={ amount }>
{ formatCurrency( amount, currency ) }
</div>
) ) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ export default {
save: ( { attributes: { rid } } ) => (
<>
{ rid.map( restaurantId => (
<a href={ `https://www.opentable.com/restref/client/?rid=${ restaurantId }` }>
<a
key={ restaurantId }
href={ `https://www.opentable.com/restref/client/?rid=${ restaurantId }` }
>
{ `https://www.opentable.com/restref/client/?rid=${ restaurantId }` }
</a>
) ) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ export default {
save: ( { attributes: { rid } } ) => (
<div>
{ rid.map( restaurantId => (
<a href={ `https://www.opentable.com/restref/client/?rid=${ restaurantId }` }>
<a
key={ restaurantId }
href={ `https://www.opentable.com/restref/client/?rid=${ restaurantId }` }
>
{ `https://www.opentable.com/restref/client/?rid=${ restaurantId }` }
</a>
) ) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export default function BlockNudge( {
// Use href to determine whether or not to display the Upgrade button.
href && [
<Button
key="nudge"
href={ href } // Only for server-side rendering, since onClick doesn't work there.
onClick={ handleClick }
target="_top"
Expand Down
5 changes: 5 additions & 0 deletions projects/plugins/protect/changelog/add-eslint-react-jsx-key
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: fixed
Comment: Add `key` to some React `actions` props. Should be no change to functionality.


1 change: 1 addition & 0 deletions projects/plugins/protect/src/js/routes/firewall/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ const FirewallPage = () => {
children={ <Text>{ __( 'Re-enable the Firewall to continue.', 'jetpack-protect' ) }</Text> }
actions={ [
<Button
key="enable"
variant="link"
onClick={ toggleWaf }
isLoading={ isUpdating }
Expand Down
3 changes: 0 additions & 3 deletions tools/js-tools/eslintrc/react.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,5 @@ module.exports = {
'react/no-did-mount-set-state': 'error',
'react/no-did-update-set-state': 'error',
'react/prefer-es6-class': 'warn',

// Temporarily override plugin:@wordpress/react so we can clean up failing stuff in separate PRs.
'react/jsx-key': 'off',
},
};

0 comments on commit 391bb78

Please sign in to comment.