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

Jump to bottom

[IconButton] Warn when providing onClick to a child of a button #14160

Merged
merged 2 commits into from Jan 13, 2019
Merged

[IconButton] Warn when providing onClick to a child of a button #14160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
efa4136
[IconButton] Warn when providing onClick to a child of a button
oliviertassinari Jan 12, 2019
79b0ea9
eps1lon's review
oliviertassinari Jan 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 11 additions & 11 deletions package.json
View file
Open in desktop
Expand Up @@ -20,36 +20,36 @@
},
"homepage": "https://material-ui.com/",
"scripts": {
"start": "yarn docs:dev",
Copy link
Member Author

Choose a reason for hiding this comment

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

alphabetical sort

"docs:dev": "rimraf node_modules/.cache/babel-loader && cross-env BABEL_ENV=docs-development next dev",
"argos": "argos upload test/regressions/screenshots/chrome --token $ARGOS_TOKEN",
"docs:api": "yarn docs:api:core && yarn docs:api:lab",
"docs:api:core": "rimraf ./pages/api && cross-env BABEL_ENV=docs-development babel-node ./docs/scripts/buildApi.js ./packages/material-ui/src ./pages/api",
"docs:api:lab": "rimraf ./pages/lab/api && cross-env BABEL_ENV=docs-development babel-node ./docs/scripts/buildApi.js ./packages/material-ui-lab/src ./pages/lab/api",
"docs:icons": "rimraf static/icons/* && babel-node ./docs/scripts/buildIcons.js",
"docs:build": "rimraf .next && cross-env NODE_ENV=production BABEL_ENV=docs-production next build",
"docs:start": "next start",
"docs:export": "rimraf docs/export && next export -o docs/export && yarn docs:build-sw && cp -r docs/static/. docs/export",
"docs:size-why": "DOCS_STATS_ENABLED=true yarn docs:build",
"docs:build-sw": "babel-node ./docs/scripts/buildServiceWorker.js",
"docs:deploy": "git push material-ui-docs master:latest",
"docs:dev": "rimraf node_modules/.cache/babel-loader && cross-env BABEL_ENV=docs-development next dev",
"docs:export": "rimraf docs/export && next export -o docs/export && yarn docs:build-sw && cp -r docs/static/. docs/export",
"docs:icons": "rimraf static/icons/* && babel-node ./docs/scripts/buildIcons.js",
"docs:size-why": "DOCS_STATS_ENABLED=true yarn docs:build",
"docs:start": "next start",
"jsonlint": "yarn --silent jsonlint:files | xargs -n1 jsonlint -q -c && echo \"jsonlint: no lint errors\"",
"jsonlint:files": "find . -name \"*.json\" | grep -v -f .eslintignore",
"lint": "eslint . --cache --report-unused-disable-directives && echo \"eslint: no lint errors\"",
"lint:fix": "eslint . --cache --fix && echo \"eslint: no lint errors\"",
"prettier": "yarn babel-node ./scripts/prettier.js",
"prettier:all": "yarn babel-node ./scripts/prettier.js write",
"size": "size-limit",
"size:why": "size-limit --why packages/material-ui/build/index.js",
"size:overhead:why": "size-limit --why ./test/size/overhead.js",
"size:why": "size-limit --why packages/material-ui/build/index.js",
"start": "yarn docs:dev",
"test": "yarn lint && yarn typescript && yarn test:coverage",
"test:unit": "cross-env NODE_ENV=test mocha 'packages/**/*.test.js' 'docs/**/*.test.js'",
"test:watch": "yarn test:unit --watch",
"test:coverage": "cross-env NODE_ENV=test BABEL_ENV=coverage nyc mocha 'packages/material-ui/**/*.test.js' 'packages/material-ui-utils/**/*.test.js' 'packages/material-ui-styles/**/*.test.js' && nyc report -r lcovonly",
"test:coverage:html": "cross-env NODE_ENV=test BABEL_ENV=coverage nyc mocha 'packages/material-ui/**/*.test.js' 'packages/material-ui-utils/**/*.test.js' 'packages/material-ui-styles/**/*.test.js' && nyc report --reporter=html",
"test:karma": "cross-env NODE_ENV=test karma start test/karma.conf.js --single-run",
"test:regressions": "webpack --config test/regressions/webpack.config.js && rimraf test/regressions/screenshots/chrome/* && vrtest run --config test/vrtest.config.js --record",
"typescript": "lerna run typescript",
"argos": "argos upload test/regressions/screenshots/chrome --token $ARGOS_TOKEN"
"test:unit": "cross-env NODE_ENV=test mocha 'packages/**/*.test.js' 'docs/**/*.test.js'",
"test:watch": "yarn test:unit --watch",
"typescript": "lerna run typescript"
},
"peerDependencies": {
"react": "*",
Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-plugin-material-ui/package.json
View file
Open in desktop
Expand Up @@ -5,7 +5,7 @@
"description": "Custom eslint rules for Material-UI.",
"main": "src/index.js",
"scripts": {
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/eslint-plugin-material-ui/**/*.test.js'"
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/eslint-plugin-material-ui/**/*.test.js' --exclude '**/node_modules/**'"
},
"repository": {
"type": "git",
Expand Down
12 changes: 6 additions & 6 deletions packages/material-ui-icons/package.json
View file
Open in desktop
Expand Up @@ -22,16 +22,16 @@
},
"homepage": "https://github.com/mui-org/material-ui/tree/master/packages/material-ui-icons",
"scripts": {
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/material-ui-icons/**/*.test.js'",
"src:download": "cd ../../ && babel-node --config-file ../../babel.config.js packages/material-ui-icons/scripts/download.js",
"src:icons": "cd ../../ && UV_THREADPOOL_SIZE=64 babel-node --config-file ../../babel.config.js packages/material-ui-icons/builder.js --output-dir packages/material-ui-icons/src --svg-dir packages/material-ui-icons/material-io-tools-icons --renameFilter ./renameFilters/material-design-icons.js",
"prebuild": "rimraf material-design-icons && rimraf build",
"build": "yarn build:es2015 && yarn build:es2015modules && yarn build:typings && yarn build:copy-files",
"build:copy-files": "babel-node --config-file ../../babel.config.js ./scripts/copy-files.js",
"build:es2015": "cross-env NODE_ENV=production babel --config-file ../../babel.config.js ./src --out-dir ./build",
"build:es2015modules": "cross-env NODE_ENV=production BABEL_ENV=modules babel --config-file ../../babel.config.js ./src/index.js --out-file ./build/index.es.js",
"build:copy-files": "babel-node --config-file ../../babel.config.js ./scripts/copy-files.js",
"build:typings": "babel-node --config-file ../../babel.config.js ./scripts/create-typings.js",
"build": "yarn build:es2015 && yarn build:es2015modules && yarn build:typings && yarn build:copy-files",
"prebuild": "rimraf material-design-icons && rimraf build",
"release": "yarn build && npm publish build",
"src:download": "cd ../../ && babel-node --config-file ../../babel.config.js packages/material-ui-icons/scripts/download.js",
"src:icons": "cd ../../ && UV_THREADPOOL_SIZE=64 babel-node --config-file ../../babel.config.js packages/material-ui-icons/builder.js --output-dir packages/material-ui-icons/src --svg-dir packages/material-ui-icons/material-io-tools-icons --renameFilter ./renameFilters/material-design-icons.js",
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/material-ui-icons/**/*.test.js' --exclude '**/node_modules/**'",
"typescript": "tslint -p tsconfig.json \"src/**/*.{ts,tsx}\""
},
"peerDependencies": {
Expand Down
10 changes: 5 additions & 5 deletions packages/material-ui-lab/package.json
View file
Open in desktop
Expand Up @@ -22,14 +22,14 @@
},
"homepage": "https://github.com/mui-org/material-ui/tree/master/packages/material-ui-lab",
"scripts": {
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/material-ui-lab/**/*.test.js'",
"prebuild": "rimraf build",
"build": "yarn build:es2015 && yarn build:es2015modules && yarn build:es && yarn build:copy-files",
"build:copy-files": "babel-node --config-file ../../babel.config.js ./scripts/copy-files.js",
"build:es": "cross-env NODE_ENV=production BABEL_ENV=es babel --config-file ../../babel.config.js ./src --out-dir ./build/es --ignore *.test.js",
"build:es2015": "cross-env NODE_ENV=production babel --config-file ../../babel.config.js ./src --out-dir ./build --ignore *.test.js",
"build:es2015modules": "cross-env NODE_ENV=production BABEL_ENV=modules babel --config-file ../../babel.config.js ./src/index.js --out-file ./build/index.es.js",
"build:es": "cross-env NODE_ENV=production BABEL_ENV=es babel --config-file ../../babel.config.js ./src --out-dir ./build/es --ignore *.test.js",
"build:copy-files": "babel-node --config-file ../../babel.config.js ./scripts/copy-files.js",
"build": "yarn build:es2015 && yarn build:es2015modules && yarn build:es && yarn build:copy-files",
"prebuild": "rimraf build",
"release": "yarn build && npm publish build",
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/material-ui-lab/**/*.test.js' --exclude '**/node_modules/**'",
"typescript": "tslint -p tsconfig.json \"src/**/*.{ts,tsx}\""
},
"peerDependencies": {
Expand Down
10 changes: 5 additions & 5 deletions packages/material-ui-styles/package.json
View file
Open in desktop
Expand Up @@ -22,14 +22,14 @@
},
"homepage": "https://github.com/mui-org/material-ui/tree/master/packages/material-ui-styles",
"scripts": {
"prebuild": "rimraf build",
"build": "yarn build:es2015 && yarn build:es2015modules && yarn build:es && yarn build:copy-files",
"build:copy-files": "babel-node --config-file ../../babel.config.js ./scripts/copy-files.js",
"build:es": "cross-env NODE_ENV=production BABEL_ENV=es babel --config-file ../../babel.config.js ./src --out-dir ./build/es --ignore *.test.js",
"build:es2015": "cross-env NODE_ENV=production babel --config-file ../../babel.config.js ./src --out-dir ./build --ignore *.test.js",
"build:es2015modules": "cross-env NODE_ENV=production BABEL_ENV=modules babel --config-file ../../babel.config.js ./src/index.js --out-file ./build/index.es.js",
"build:es": "cross-env NODE_ENV=production BABEL_ENV=es babel --config-file ../../babel.config.js ./src --out-dir ./build/es --ignore *.test.js",
"build:copy-files": "babel-node --config-file ../../babel.config.js ./scripts/copy-files.js",
"build": "yarn build:es2015 && yarn build:es2015modules && yarn build:es && yarn build:copy-files",
"prebuild": "rimraf build",
"release": "yarn build && npm publish build",
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/material-ui-styles/**/*.test.js'",
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/material-ui-styles/**/*.test.js' --exclude '**/node_modules/**'",
"typescript": "tslint -p tsconfig.json \"{src,test}/**/*.{ts,tsx}\""
},
"peerDependencies": {
Expand Down
10 changes: 5 additions & 5 deletions packages/material-ui-system/package.json
View file
Open in desktop
Expand Up @@ -22,14 +22,14 @@
},
"homepage": "https://github.com/mui-org/material-ui/tree/master/packages/material-ui-system",
"scripts": {
"prebuild": "rimraf build",
"build": "yarn build:es2015 && yarn build:es2015modules && yarn build:es && yarn build:copy-files",
"build:copy-files": "babel-node --config-file ../../babel.config.js ./scripts/copy-files.js",
"build:es": "cross-env NODE_ENV=production BABEL_ENV=es babel --config-file ../../babel.config.js ./src --out-dir ./build/es --ignore *.test.js",
"build:es2015": "cross-env NODE_ENV=production babel --config-file ../../babel.config.js ./src --out-dir ./build --ignore *.test.js",
"build:es2015modules": "cross-env NODE_ENV=production BABEL_ENV=modules babel --config-file ../../babel.config.js ./src/index.js --out-file ./build/index.es.js",
"build:es": "cross-env NODE_ENV=production BABEL_ENV=es babel --config-file ../../babel.config.js ./src --out-dir ./build/es --ignore *.test.js",
"build:copy-files": "babel-node --config-file ../../babel.config.js ./scripts/copy-files.js",
"build": "yarn build:es2015 && yarn build:es2015modules && yarn build:es && yarn build:copy-files",
"prebuild": "rimraf build",
"release": "yarn build && npm publish build",
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/material-ui-system/**/*.test.js'"
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/material-ui-system/**/*.test.js' --exclude '**/node_modules/**'"
},
"peerDependencies": {
"react": "^16.3.0",
Expand Down
10 changes: 5 additions & 5 deletions packages/material-ui-utils/package.json
View file
Open in desktop
Expand Up @@ -22,14 +22,14 @@
},
"homepage": "https://github.com/mui-org/material-ui/tree/master/packages/material-ui-utils",
"scripts": {
"prebuild": "rimraf build",
"build": "yarn build:es2015 && yarn build:es2015modules && yarn build:es && yarn build:copy-files",
"build:copy-files": "babel-node --config-file ../../babel.config.js ./scripts/copy-files.js",
"build:es": "cross-env NODE_ENV=production BABEL_ENV=es babel --config-file ../../babel.config.js ./src --out-dir ./build/es --ignore *.test.js",
"build:es2015": "cross-env NODE_ENV=production babel --config-file ../../babel.config.js ./src --out-dir ./build --ignore *.test.js",
"build:es2015modules": "cross-env NODE_ENV=production BABEL_ENV=modules babel --config-file ../../babel.config.js ./src/index.js --out-file ./build/index.es.js",
"build:es": "cross-env NODE_ENV=production BABEL_ENV=es babel --config-file ../../babel.config.js ./src --out-dir ./build/es --ignore *.test.js",
"build:copy-files": "babel-node --config-file ../../babel.config.js ./scripts/copy-files.js",
"build": "yarn build:es2015 && yarn build:es2015modules && yarn build:es && yarn build:copy-files",
"prebuild": "rimraf build",
"release": "yarn build && npm publish build",
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/material-ui-utils/**/*.test.js'"
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/material-ui-utils/**/*.test.js' --exclude '**/node_modules/**'"
},
"peerDependencies": {
"react": "^16.3.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/package.json
View file
Open in desktop
Expand Up @@ -28,7 +28,7 @@
"build:copy-files": "babel-node --config-file ../../babel.config.js ./scripts/copy-files.js",
"build": "yarn build:es2015 && yarn build:es2015modules && yarn build:es && yarn build:umd && yarn build:copy-files",
"release": "yarn build && npm publish build",
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/material-ui/**/*.test.js'",
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/material-ui/**/*.test.js' --exclude '**/node_modules/**'",
"typescript": "tslint -p tsconfig.json \"{src,test}/**/*.{ts,tsx}\""
},
"peerDependencies": {
Expand Down
24 changes: 23 additions & 1 deletion packages/material-ui/src/IconButton/IconButton.js
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@
import React from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';
import { chainPropTypes } from '@material-ui/utils';
import withStyles from '../styles/withStyles';
import { fade } from '../styles/colorManipulator';
import ButtonBase from '../ButtonBase';
Expand Down Expand Up @@ -103,7 +104,28 @@ IconButton.propTypes = {
/**
* The icon element.
*/
children: PropTypes.node,
children: chainPropTypes(PropTypes.node, props => {
const found = React.Children.toArray(props.children).some(child => {
return React.isValidElement(child) && child.props.onClick;
});

if (found) {
return new Error(
[
'Material-UI: you are providing an onClick event listener ' +
'to a child of a button element.',
'Firefox will never trigger the event.',
'You should move the onClick listener to the parent button element.',
'https://github.com/mui-org/material-ui/issues/13957',
// Change error message slightly on every check to prevent caching when testing
// which would not trigger console errors on subsequent fails
process.env.NODE_ENV === 'test' ? Date.now() : '',
].join('\n'),
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
);
}

return null;
}),
/**
* Override or extend the styles applied to the component.
* See [CSS API](#css-api) below for more details.
Expand Down
21 changes: 21 additions & 0 deletions packages/material-ui/src/IconButton/IconButton.test.js
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@ import { spy } from 'sinon';
import { assert } from 'chai';
import PropTypes from 'prop-types';
import { createShallow, createMount, getClasses } from '@material-ui/core/test-utils';
import consoleErrorMock from 'test/utils/consoleErrorMock';
import Icon from '../Icon';
import ButtonBase from '../ButtonBase';
import IconButton from './IconButton';
Expand Down Expand Up @@ -107,4 +108,24 @@ describe('<IconButton />', () => {
assert.strictEqual(ReactDOM.findDOMNode(ref.args[0][0]).type, 'button');
});
});

describe('Firefox onClick', () => {
beforeEach(() => {
consoleErrorMock.spy();
});

afterEach(() => {
consoleErrorMock.reset();
});

it('should raise a warning', () => {
mount(
<IconButton>
<svg onClick={() => {}} />
</IconButton>,
);
assert.strictEqual(consoleErrorMock.callCount(), 1);
assert.include(consoleErrorMock.args()[0][0], 'you are providing an onClick event listener');
});
});
});