From a4891d22b070ef21ec301fb30ef3c34c92f968f8 Mon Sep 17 00:00:00 2001 From: DD Liu Date: Thu, 27 Jul 2017 00:34:33 -0400 Subject: [PATCH] code review --- .babelrc | 1 + package.json | 8 +++++--- src/components/paint-editor.jsx | 5 ++--- src/containers/paint-editor.jsx | 21 ++++++++++----------- src/containers/paper-canvas.jsx | 20 +++++++++----------- src/log/log.js | 2 +- src/playground/playground.jsx | 7 ++----- src/reducers/tools.js | 9 +++------ src/tools/tool-types.js | 16 ++++++---------- test/unit/tools-reducer.test.js | 2 +- 10 files changed, 40 insertions(+), 51 deletions(-) diff --git a/.babelrc b/.babelrc index facd1809..dc76ced0 100644 --- a/.babelrc +++ b/.babelrc @@ -1,3 +1,4 @@ { + "plugins": ["transform-object-rest-spread"], "presets": ["es2015", "react"] } \ No newline at end of file diff --git a/package.json b/package.json index 2c5130de..ba1ba054 100644 --- a/package.json +++ b/package.json @@ -9,7 +9,7 @@ "deploy": "touch playground/.nojekyll && gh-pages -t -d playground -m \"Build for $(git log --pretty=format:%H -n1)\"", "lint": "eslint . --ext .js,.jsx", "start": "webpack-dev-server", - "test": "npm run lint && npm run build && npm run unit-test", + "test": "npm run lint && npm run build && npm run unit", "unit": "jest", "watch": "webpack --progress --colors --watch" }, @@ -44,15 +44,17 @@ "gh-pages": "github:rschamp/gh-pages#publish-branch-to-subfolder", "html-webpack-plugin": "2.28.0", "jest": "^20.0.4", + "keymirror": "0.1.1", + "lodash.bindall": "4.4.0", "lodash.defaultsdeep": "4.6.0", "minilog": "3.1.0", "mkdirp": "^0.5.1", - "paper": "^0.11.4", + "paper": "0.11.4", "postcss-import": "^10.0.0", "postcss-loader": "^2.0.5", "postcss-simple-vars": "^4.0.0", "prop-types": "^15.5.10", - "react": "15.5.4", + "react": "15.6.1", "react-dom": "15.5.4", "react-intl": "2.3.0", "react-redux": "5.0.5", diff --git a/src/components/paint-editor.jsx b/src/components/paint-editor.jsx index a22a01e6..4c3d025c 100644 --- a/src/components/paint-editor.jsx +++ b/src/components/paint-editor.jsx @@ -1,6 +1,7 @@ import PropTypes from 'prop-types'; import React from 'react'; import PaperCanvas from '../containers/paper-canvas.jsx'; +import ToolTypes from '../tools/tool-types.js'; const PaintEditorComponent = props => ( ( ); PaintEditorComponent.propTypes = { - tool: PropTypes.shape({ - name: PropTypes.string.isRequired - }) + tool: PropTypes.oneOf(Object.keys(ToolTypes)).isRequired }; export default PaintEditorComponent; diff --git a/src/containers/paint-editor.jsx b/src/containers/paint-editor.jsx index 4e219058..40cc74e4 100644 --- a/src/containers/paint-editor.jsx +++ b/src/containers/paint-editor.jsx @@ -7,26 +7,25 @@ import {connect} from 'react-redux'; class PaintEditor extends React.Component { componentDidMount () { - const onKeyPress = this.props.onKeyPress; - document.onkeydown = function (e) { - e = e || window.event; - onKeyPress(e); - }; + document.addEventListener('keydown', this.props.onKeyPress); + } + componentWillUnmount () { + document.removeEventListener('keydown', this.props.onKeyPress); } render () { + const { + onKeyPress, // eslint-disable-line no-unused-vars + ...props + } = this.props; return ( - + ); } } PaintEditor.propTypes = { onKeyPress: PropTypes.func.isRequired, - tool: PropTypes.shape({ - name: PropTypes.string.isRequired - }) + tool: PropTypes.oneOf(Object.keys(ToolTypes)).isRequired }; const mapStateToProps = state => ({ diff --git a/src/containers/paper-canvas.jsx b/src/containers/paper-canvas.jsx index 5eb3a9aa..a3316dbf 100644 --- a/src/containers/paper-canvas.jsx +++ b/src/containers/paper-canvas.jsx @@ -4,13 +4,8 @@ import paper from 'paper'; import ToolTypes from '../tools/tool-types.js'; class PaperCanvas extends React.Component { - constructor (props) { - super(props); - this.state = { - }; - } componentDidMount () { - paper.setup('paper-canvas'); + paper.setup(this.canvas); // Create a Paper.js Path to draw a line into it: const path = new paper.Path(); // Give the stroke a color @@ -25,23 +20,26 @@ class PaperCanvas extends React.Component { paper.view.draw(); } componentWillReceiveProps (nextProps) { - if (nextProps.tool !== this.props.tool && nextProps.tool instanceof ToolTypes) { + if (nextProps.tool !== this.props.tool) { // TODO switch tool } } componentWillUnmount () { + paper.remove(); } render () { return ( - + { + this.canvas = canvas; + }} + /> ); } } PaperCanvas.propTypes = { - tool: PropTypes.shape({ - name: PropTypes.string.isRequired - }) + tool: PropTypes.oneOf(Object.keys(ToolTypes)).isRequired }; export default PaperCanvas; diff --git a/src/log/log.js b/src/log/log.js index 5ea3bb07..91414157 100644 --- a/src/log/log.js +++ b/src/log/log.js @@ -1,4 +1,4 @@ import minilog from 'minilog'; minilog.enable(); -export default minilog('paint-editor'); +export default minilog('scratch-paint'); diff --git a/src/playground/playground.jsx b/src/playground/playground.jsx index 31ad10cf..8a27161e 100644 --- a/src/playground/playground.jsx +++ b/src/playground/playground.jsx @@ -2,15 +2,12 @@ import React from 'react'; import ReactDOM from 'react-dom'; import PaintEditor from '..'; import {Provider} from 'react-redux'; -import {createStore, applyMiddleware} from 'redux'; -import throttle from 'redux-throttle'; +import {createStore} from 'redux'; import reducer from '../reducers/combine-reducers'; const appTarget = document.createElement('div'); document.body.appendChild(appTarget); -const store = applyMiddleware( - throttle(300, {leading: true, trailing: true}) -)(createStore)( +const store = createStore( reducer, window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__() ); diff --git a/src/reducers/tools.js b/src/reducers/tools.js index 28832833..57b7d4c7 100644 --- a/src/reducers/tools.js +++ b/src/reducers/tools.js @@ -8,10 +8,10 @@ const reducer = function (state, action) { if (typeof state === 'undefined') state = initialState; switch (action.type) { case CHANGE_TOOL: - if (action.tool instanceof ToolTypes) { + if (action.tool in ToolTypes) { return action.tool; } - log.warn(`Warning: Tool type does not exist: ${action.tool}`); + log.warn(`Tool type does not exist: ${action.tool}`); /* falls through */ default: return state; @@ -22,10 +22,7 @@ const reducer = function (state, action) { reducer.changeTool = function (tool) { return { type: CHANGE_TOOL, - tool: tool, - meta: { - throttle: 30 - } + tool: tool }; }; diff --git a/src/tools/tool-types.js b/src/tools/tool-types.js index 61524cb7..9a850d80 100644 --- a/src/tools/tool-types.js +++ b/src/tools/tool-types.js @@ -1,12 +1,8 @@ -class ToolTypes { - constructor (name) { - this.name = name; - } - toString () { - return `ToolTypes.${this.name}`; - } -} -ToolTypes.BRUSH = new ToolTypes('BRUSH'); -ToolTypes.ERASER = new ToolTypes('ERASER'); +import keyMirror from 'keymirror'; + +const ToolTypes = keyMirror({ + BRUSH: null, + ERASER: null +}); export default ToolTypes; diff --git a/test/unit/tools-reducer.test.js b/test/unit/tools-reducer.test.js index f0c729f2..428b3118 100644 --- a/test/unit/tools-reducer.test.js +++ b/test/unit/tools-reducer.test.js @@ -4,7 +4,7 @@ import reducer from '../../src/reducers/tools'; test('initialState', () => { let defaultState; - expect(reducer(defaultState /* state */, {type: 'anything'} /* action */) instanceof ToolTypes).toBeTruthy(); + expect(reducer(defaultState /* state */, {type: 'anything'} /* action */) in ToolTypes).toBeTruthy(); }); test('changeTool', () => {