Refactor Editor/index.jsx to functional React component #3309
Conversation
|
@raclim Can you review this . Please |
There was a problem hiding this comment.
Thanks so much for giving this a shot!
Unfortunately I don't think we'll be able to accept this pull request as we're currently reworking this component for a project we started earlier last year, which was upgrading the CodeMirror version.
However, I think this is a great start and gave a few notes on a few changes I might make on this pull request! I think one additional note I'd make is that the component does not appear to be fully functional yet, and crashes when attempting to save a sketch. We strongly encourage doing both unit and manual testing on your code before submitting a pull request!
I'm sorry that we couldn't get this one in, but please feel free to check out our other issues.
| @@ -3,6 +3,7 @@ const path = require('path'); | |||
| const CopyWebpackPlugin = require('copy-webpack-plugin'); | |||
| const ReactRefreshPlugin = require('@pmmmwh/react-refresh-webpack-plugin'); | |||
| const ESLintPlugin = require('eslint-webpack-plugin'); | |||
| const NodePolyfillPlugin = require('node-polyfill-webpack-plugin'); | |||
There was a problem hiding this comment.
Ideally, we would like to isolate changes in each pull request to relate as much as possible to the issue. It seems like the changes in this file aren't related to the Editor component refactor, so I might remove these!
| @@ -113,6 +113,7 @@ | |||
| "babel-core": "^7.0.0-bridge.0", | |||
| "babel-loader": "^8.2.5", | |||
| "babel-plugin-transform-react-remove-prop-types": "^0.2.12", | |||
| "browserify-zlib": "^0.2.0", | |||
There was a problem hiding this comment.
Same here as well with removing changes that may not be related to the issue!
| @@ -0,0 +1,716 @@ | |||
| // TODO: convert to functional component | |||
There was a problem hiding this comment.
In order to help keep the repository directory easy to navigate, ideally we would not create a new file holding the component, and would just make our changes directly to the index.jsx file.
Because of the way Git works, the history would be preserved in case we would ever want to look back at the old component!
| } | ||
| }, [props.file.id, props.file.name, props.unsavedChanges]); | ||
|
|
||
| // useEffect(() => { |
There was a problem hiding this comment.
To maintain readability, we'd ideally only want to include comments that provide supportive information about the code!
Refactored : Todo - Convert the Editor/index.jsx to React functional component
Changes: Added a new file called "oldClassComponent.jsx" so original class component remains there. Replaced index.jsx with new functional component code.
Video- https://drive.google.com/file/d/1iFMe2Cc_KBI9-QoaQGz8gJU7N473UGZa/view?usp=sharing
I have verified that this pull request:
npm run lint)npm run test)developbranch.Fixes #123