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

Add the default eslint react hooks config to the wordpress eslint package #14995

Merged
merged 2 commits into from
Apr 16, 2019
Merged

Add the default eslint react hooks config to the wordpress eslint package #14995

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8bce4c
Add the default eslint react hooks config to the wordpress eslint pac…
youknowriad Apr 16, 2019
4f3a05a
Update the contrast checker snapshots
youknowriad Apr 16, 2019
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
7 changes: 7 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 23 additions & 11 deletions packages/block-editor/src/components/contrast-checker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,22 @@ import { __ } from '@wordpress/i18n';
import { Notice } from '@wordpress/components';
import { useEffect } from '@wordpress/element';

function ContrastCheckerMessage( { tinyBackgroundColor, tinyTextColor, backgroundColor, textColor } ) {
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, what ESLint rule was triggered which enforced this refactor? I'm wondering what is the lesson learned.

Copy link
Contributor Author

@youknowriad youknowriad Apr 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The reason is that you can't use hooks after "early returns"

const msg = tinyBackgroundColor.getBrightness() < tinyTextColor.getBrightness() ?
__( 'This color combination may be hard for people to read. Try using a darker background color and/or a brighter text color.' ) :
__( 'This color combination may be hard for people to read. Try using a brighter background color and/or a darker text color.' );
useEffect( () => {
speak( __( 'This color combination may be hard for people to read.' ) );
}, [ backgroundColor, textColor ] );
return (
<div className="editor-contrast-checker block-editor-contrast-checker">
<Notice status="warning" isDismissible={ false }>
{ msg }
</Notice>
</div>
);
}

function ContrastChecker( {
backgroundColor,
fallbackBackgroundColor,
Expand All @@ -33,18 +49,14 @@ function ContrastChecker( {
) ) {
return null;
}
const msg = tinyBackgroundColor.getBrightness() < tinyTextColor.getBrightness() ?
__( 'This color combination may be hard for people to read. Try using a darker background color and/or a brighter text color.' ) :
__( 'This color combination may be hard for people to read. Try using a brighter background color and/or a darker text color.' );
useEffect( () => {
speak( __( 'This color combination may be hard for people to read.' ) );
}, [ backgroundColor, textColor ] );

return (
<div className="editor-contrast-checker block-editor-contrast-checker">
<Notice status="warning" isDismissible={ false }>
{ msg }
</Notice>
</div>
<ContrastCheckerMessage
backgroundColor={ backgroundColor }
textColor={ textColor }
tinyBackgroundColor={ tinyBackgroundColor }
tinyTextColor={ tinyTextColor }
/>
);
}

Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### New Features

- New Rule: [`@wordpress/react-no-unsafe-timeout`](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/react-no-unsafe-timeout.md)
- Add [React Hooks Rules](https://reactjs.org/docs/hooks-rules.html) config.

## 2.1.0 (2019-03-20)

Expand Down
3 changes: 3 additions & 0 deletions packages/eslint-plugin/configs/react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ module.exports = {
},
plugins: [
'react',
'react-hooks',
],
rules: {
'react/display-name': 'off',
Expand All @@ -24,5 +25,7 @@ module.exports = {
'react/no-children-prop': 'off',
'react/prop-types': 'off',
'react/react-in-jsx-scope': 'off',
'react-hooks/rules-of-hooks': 'error',
Copy link
Member

@gziolo gziolo Apr 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting, they don't provide the recommended preset.

'react-hooks/exhaustive-deps': 'warn',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hesitant on the need of this one as we already have a valid use-case for extra deps, but I'm merging and we can try and see if it feels constraining.

},
};
1 change: 1 addition & 0 deletions packages/eslint-plugin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
"babel-eslint": "^8.0.3",
"eslint-plugin-jsx-a11y": "^6.2.1",
"eslint-plugin-react": "^7.12.4",
"eslint-plugin-react-hooks": "^1.6.0",
"requireindex": "^1.2.0"
},
"publishConfig": {
Expand Down