-
Notifications
You must be signed in to change notification settings - Fork 137
Moving code base to React #550
Conversation
Move all js in app folder, and keep static files next to it. Current version still work without react. Webpack is integrated, but not used yet.
No redux yet, and no refactoring.
|
It'd be nice to also add the eslint-plugin-react module and extend the "plugin:react/recommended" config. |
AFAIK, we are using JSX. |
.gitignore
Outdated
| test/dist/* | ||
| ckeditor-build/build/* | ||
| build/ | ||
| .DS_Store |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add this to your global git ignore on your machine, not this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! should have been done a long time ago, thanks
| { | ||
| "name": "notes", | ||
| "version": "2.2.0dev", | ||
| "lockfileVersion": 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we are using a package-lock, could we please specify the npm version required for this project in package.json, see example at https://docs.npmjs.com/files/package.json#engines
src/sidebar/app/components/Footer.js
Outdated
| * if false, animate to savingLayout (sync icon on left) | ||
| * @param {Boolean} warning Apply yellow warning styling on toolbar | ||
| */ | ||
| function setAnimation( animateSyncIcon = true, syncingLayout, warning ) { // animateSyncIcon, syncingLayout, warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we please remove the extra spaces between args here, as above there are no spaces in formatFooterTime(date) between ( ) and date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prettier should be used to make sure this is no longer a problem. I guess some configurations are needed. So you think travis should fail pull request if prettier return syntax error ?
src/sidebar/app/components/Footer.js
Outdated
| footerButtons.classList.replace('savingLayout', 'syncingLayout'); | ||
| enableSync.style.backgroundColor = 'transparent'; | ||
| // Start blink animation on saving-indicator | ||
| savingIndicator.classList.add('blink'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we gonna use any react magic for this animation or leave it like it is right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First step was migration, refactoring is coming 👍
| const SURVEY_PATH = 'https://qsurvey.mozilla.com/s3/notes?ref=sidebar'; | ||
|
|
||
|
|
||
| class Footer extends React.Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functions above for this component make this component file really long and complex, can we move the utils above out of here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, refactoring is coming.
| import React from 'react'; | ||
|
|
||
| import Editor from './Editor'; | ||
| import Footer from './Footer'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 excellent style here for separating libs and local modules
webpack.config.js
Outdated
| {from: path.join('src')} | ||
| ], { | ||
| ignore: [ | ||
| '.DS_Store', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is strange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't remember why it's here. Has been removed.
webpack.config.js
Outdated
| @@ -0,0 +1,52 @@ | |||
| /** | |||
| * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably remove these headers, isn't this our webpack file now?
webpack.watch.js
Outdated
| @@ -0,0 +1,10 @@ | |||
| /** | |||
| * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also can be removed?
| const CopyWebpackPlugin = require('copy-webpack-plugin'); | ||
|
|
||
| module.exports = { | ||
| devtool: 'source-map', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a source-map because i don't think we are minifying anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look on that, but Babel compile jsx files into js during build process. My guess is that sourcemap is needed for debugging and identify correct files or lines of code.
|
This pull request was the result of a one day sprint with Natim to answer some questions regarding integrating React and having a working prototype. Will spend next few days finishing the integration and have a clean merge to do, thanks everyone for your your feedbacks 👍 |
|
yaaaay!!!!! |
src/sidebar/app/components/Footer.js
Outdated
| enableSync.style.backgroundColor = 'transparent'; | ||
| // Start blink animation on saving-indicator | ||
| savingIndicator.classList.add('blink'); | ||
| // Reset CSS animation by removeing class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "removing"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just fixed it.
|
I made a few tweaks, but this seems to get ESLint a bit happier (note changes in env:
browser: true
es6: true
webextensions: true
parserOptions:
ecmaFeatures:
jsx: true
ecmaVersion: 2017
sourceType: module
extends:
- eslint:recommended
- plugin:react/recommended
- plugin:jsx-a11y/recommended
globals:
BrowserStorageCredentials: false
ClassicEditor: false
Jose: false
JoseJWE: false
Kinto: false
KintoClient: false
Quill: false
TestPilotGA: false
customizeEditor: false
disconnectFromKinto: false
formatFooterTime: false
fxaCryptoRelier: false
fxaFetchProfile: false
fxaRenewCredential: false
getPadStats: false
loadFromKinto: false
localizeEditorButtons: false
migrationCheck: false
retrieveNote: false
saveToKinto: false
setAnimation: false
plugins:
- jsx-a11y
- react
root: true
rules:
eqeqeq: error
linebreak-style: [error, unix]
no-console: warn
no-var: error
prefer-const: error
quotes: [error, single]
semi: [error, always](And we'd need to add the new ESLint plugins — |
|
Travis-CI is bombing with the following:
Oddly, I also saw that locally and had to manually install the dependency. ¯\_(ツ)_/¯ |
|
Also curious, I get a Webpack error when I try running $ npm run build-ck
> notes@2.2.0-dev build-ck /Users/pdehaan/dev/github/mozilla/notes
> cd ckeditor-build && webpack && cd ..
Hash: 79574a7e2d83670383b1
Version: webpack 3.10.0
Time: 19750ms
Asset Size Chunks Chunk Names
ckeditor.js 351 kB 0 [emitted] [big] main
ckeditor.js.map 2.12 MB 0 [emitted] main
[255] ./src/ckeditor.js 1.5 kB {0} [built]
[460] ./src/strike.js 1.47 kB {0} [built]
[461] ./src/strikeengine.js 1.86 kB {0} [built]
[462] ./src/icons/strike.svg 1.3 kB {0} [built]
+ 476 hidden modules
ERROR in ./src/ckeditor.js
Module not found: Error: Can't resolve '@ckeditor/ckeditor5-autoformat/src/autoformat' in '/Users/pdehaan/dev/github/mozilla/notes/ckeditor-build/src'
@ ./src/ckeditor.js 8:0-77
ERROR in ./src/ckeditor.js
Module not found: Error: Can't resolve '@ckeditor/ckeditor5-heading/src/heading' in '/Users/pdehaan/dev/github/mozilla/notes/ckeditor-build/src'
@ ./src/ckeditor.js 14:0-68I was trying to figure out if running |
Mostly refacotring syntax based on new eslint config file. However code is currently broken, will be fixed in next commit
src/sidebar/app/panel.js
Outdated
| @@ -0,0 +1,288 @@ | |||
| /* exported TEXT_ALIGN_DIR */ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just delete this file? It looks 100% commented out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did split content between Editor and Footer component, commenting section by section while progressing. Main reason I kept it is because the TEXT_ALIGN_DIR seams not used anymore and I want to do more test. Will be removed yes.
src/sidebar/index.html
Outdated
| </body> | ||
| <script src="vendor/ckeditor.js"></script> | ||
| <script defer src="vendor/material.js"></script> | ||
| <script src="app.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the <script> tags should probably be within the <body> tag, instead of orphaned here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until yesteday, we were powering react directly on body, which clear its content and explain why script were after body. That's bad practice and has been changed now using a div. I will move those back inside body, thanks.
src/sync.js
Outdated
| */ | ||
| function loadFromKinto(client, credentials) { | ||
| function loadFromKinto(client, credentials) { // eslint-disable-line no-unused-vars | ||
| console.log('loadFromKinto', client, credentials); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: not sure if we want to remove these debugging console.log() messages (here and L293 below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆, definitely should be removed. Was debugging late and and forgot those, thanks 👍
| "devDependencies": { | ||
| "babel-core": "^6.26.0", | ||
| "babel-loader": "^7.1.2", | ||
| "babel-minify-webpack-plugin": "^0.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in ckeditor-build/webpack.config.js, so don't really know much about it but was not part of this PR.
Should we create an issue to try simplifing ckeditor build process ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, to avoid this in the future hopefully we can merge the two webpack files together :)
|
|
||
| import Panel from './components/Panel'; | ||
|
|
||
| import '../static/scss/styles.scss'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| <p>${browser.i18n.getMessage('welcomeThatsIt')}</p> | ||
| `.replace(/(?:\n(?:\s*))+/g, '').trim(); | ||
|
|
||
| export default INITIAL_CONTENT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 clean
src/sidebar/static/scss/footer.scss
Outdated
| .savingLayout #give-feedback-button { | ||
| opacity: 1; | ||
| z-index: 1; | ||
| visiblity: visible; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo here, maybe we had this before as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next line was also very funny: transition: visibity 0s var(--sync-animation-duration); 🤣. Has been fixed, was messing up with animations, thanks for finding it.
src/sidebar/static/scss/footer.scss
Outdated
| opacity: 0; | ||
| z-index: 0; | ||
| cursor: default; | ||
| visiblity: hidden; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same typo as mentioned above
| { | ||
| test: /\.js$/, | ||
| exclude: /node_modules/, | ||
| use: ['babel-loader?presets[]=env&presets[]=react&presets[]=stage-2&sourceMaps=true'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What issue does react cause if we do not use babel? i.e What is not supported in FF that we had to use it?
I'm all for using it just wondering, maybe worth adding a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Babel compile jsx syntax to js. Sound confusing since we store jsx code in js files (could be .jsx to be more explicit but react use .js by default). I will add a comment 👍.
package.json
Outdated
| "test:karma": "webpack --config webpack.test-unit.js && NODE_ENV=test karma start" | ||
| "test:karma": "webpack --config webpack.test-unit.js && NODE_ENV=test karma start", | ||
| "webpack": "webpack --config webpack.config.js", | ||
| "webpack:watch": "webpack --config webpack.watch.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need webpack.watch.js file or can u just tweak this command and add --watch ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're perfectly right. Can't remember why we did it like this (I guess had extra config for this watch case during refactoring), but definitely can be remove now.
|
First version using React : notes_2.3.0dev1.zip |
|
|
New version for testing: notes_2.3.0dev2.zip |
|
I am shipping this and we will follow up on smaller PR if needed. 🇫🇷 Lâchez vos comm!! |
|
wowowo! |
|
@pdehaan I'm running into a similar situation as outlined in your comment here where I can't build ckeditor using I opened an issue with my console error output, see #798. |
|
@cedricium I just did an $ npm cache clean, $ nvm use 8, $ rm -rf node_modules package-lock.json, and $ npm install against latest master branch but I'm still getting errors. Not sure how CI is passing, or if it's using some cached version of the dependencies and stuff has changed recently with regards to file locations, etc. But I'm getting the same output from $ npm install (with it's > cd ckeditor-build && webpack && cd ..
Hash: 7dfdab6d45a0a2cd2e2c
Version: webpack 3.11.0
Time: 26467ms
Asset Size Chunks Chunk Names
ckeditor.js 364 kB 0 [emitted] [big] main
ckeditor.js.map 2.32 MB 0 [emitted] main
[260] ./src/ckeditor.js 1.5 kB {0} [built]
[481] ./src/strike.js 1.47 kB {0} [built]
[482] ./src/strikeengine.js 1.86 kB {0} [built]
[483] ./src/icons/strike.svg 1.3 kB {0} [built]
+ 506 hidden modules
ERROR in ../node_modules/css-loader??ref--1-1!../node_modules/sass-loader/lib/loader.js!../node_modules/@ckeditor/ckeditor5-heading/theme/theme.scss
Module build failed:
@import '~@ckeditor/ckeditor5-theme-lark/theme/helpers/_spacing';
^
File to import not found or unreadable: ~@ckeditor/ckeditor5-theme-lark/theme/helpers/_spacing.
in /Users/pdehaan/dev/github/mozilla/notes/node_modules/@ckeditor/ckeditor5-heading/theme/theme.scss (line 4, column 1)
@ ../node_modules/@ckeditor/ckeditor5-heading/theme/theme.scss 4:14-113
@ ../node_modules/@ckeditor/ckeditor5-heading/src/heading.js
@ ./src/ckeditor.js
ERROR in ./src/strikeengine.js
Module not found: Error: Can't resolve '@ckeditor/ckeditor5-engine/src/conversion/buildmodelconverter' in '/Users/pdehaan/dev/github/mozilla/notes/ckeditor-build/src'
@ ./src/strikeengine.js 11:0-96
@ ./src/strike.js
@ ./src/ckeditor.js
ERROR in ../node_modules/@ckeditor/ckeditor5-heading/src/headingengine.js
Module not found: Error: Can't resolve '@ckeditor/ckeditor5-engine/src/conversion/buildmodelconverter' in '/Users/pdehaan/dev/github/mozilla/notes/node_modules/@ckeditor/ckeditor5-heading/src'
@ ../node_modules/@ckeditor/ckeditor5-heading/src/headingengine.js 11:0-96
@ ../node_modules/@ckeditor/ckeditor5-heading/src/heading.js
@ ./src/ckeditor.js
ERROR in ./src/strikeengine.js
Module not found: Error: Can't resolve '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter' in '/Users/pdehaan/dev/github/mozilla/notes/ckeditor-build/src'
@ ./src/strikeengine.js 12:0-94
@ ./src/strike.js
@ ./src/ckeditor.js
ERROR in ../node_modules/@ckeditor/ckeditor5-heading/src/headingengine.js
Module not found: Error: Can't resolve '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter' in '/Users/pdehaan/dev/github/mozilla/notes/node_modules/@ckeditor/ckeditor5-heading/src'
@ ../node_modules/@ckeditor/ckeditor5-heading/src/headingengine.js 12:0-94
@ ../node_modules/@ckeditor/ckeditor5-heading/src/heading.js
@ ./src/ckeditor.js
ERROR in ../node_modules/@ckeditor/ckeditor5-heading/src/heading.js
Module not found: Error: Can't resolve '@ckeditor/ckeditor5-ui/src/dropdown/list/createlistdropdown' in '/Users/pdehaan/dev/github/mozilla/notes/node_modules/@ckeditor/ckeditor5-heading/src'
@ ../node_modules/@ckeditor/ckeditor5-heading/src/heading.js 12:0-93
@ ./src/ckeditor.js
ERROR in ../node_modules/@ckeditor/ckeditor5-ui/theme/components/toolbar/toolbar.css
Module parse failed: Unexpected character '@' (6:0)
You may need an appropriate loader to handle this file type.
| */
|
| @import "../../mixins/_unselectable.css";
|
| .ck-toolbar {
@ ../node_modules/@ckeditor/ckeditor5-ui/src/toolbar/toolbarview.js 18:0-52
@ ../node_modules/@ckeditor/ckeditor5-editor-classic/src/classiceditoruiview.js
@ ../node_modules/@ckeditor/ckeditor5-editor-classic/src/classiceditor.js
@ ./src/ckeditor.js
ERROR in ../node_modules/@ckeditor/ckeditor5-ui/theme/components/button/button.css
Module parse failed: Unexpected character '@' (6:0)
You may need an appropriate loader to handle this file type.
| */
|
| @import "../../mixins/_unselectable.css";
| @import "../tooltip/mixins/_tooltip.css";
|
@ ../node_modules/@ckeditor/ckeditor5-ui/src/button/buttonview.js 16:0-50
@ ./src/strike.js
@ ./src/ckeditor.js
ERROR in ../node_modules/@ckeditor/ckeditor5-ui/theme/globals/globals.css
Module parse failed: Unexpected character '@' (6:0)
You may need an appropriate loader to handle this file type.
| */
|
| @import "./_hidden.css";
| @import "./_reset.css";
| @import "./_zindex.css";
@ ../node_modules/@ckeditor/ckeditor5-ui/src/view.js 19:0-38
@ ../node_modules/@ckeditor/ckeditor5-ui/src/button/buttonview.js
@ ./src/strike.js
@ ./src/ckeditor.js
ERROR in ../node_modules/@ckeditor/ckeditor5-basic-styles/theme/code.css
Module parse failed: Unexpected token (6:0)
You may need an appropriate loader to handle this file type.
| */
|
| .ck-content code {
| background-color: hsla(0, 0%, 78%, 0.3);
| padding: .15em;
@ ../node_modules/@ckeditor/ckeditor5-basic-styles/src/code.js 14:0-27
@ ./src/ckeditor.js
ERROR in ../node_modules/@ckeditor/ckeditor5-ui/theme/components/panel/stickypanel.css
Module parse failed: Unexpected token (6:0)
You may need an appropriate loader to handle this file type.
| */
|
| .ck-editor {
| & .ck-sticky-panel {
| & .ck-sticky-panel__content_sticky {
@ ../node_modules/@ckeditor/ckeditor5-ui/src/panel/sticky/stickypanelview.js 15:0-57
@ ../node_modules/@ckeditor/ckeditor5-editor-classic/src/classiceditoruiview.js
@ ../node_modules/@ckeditor/ckeditor5-editor-classic/src/classiceditor.js
@ ./src/ckeditor.js
ERROR in ../node_modules/@ckeditor/ckeditor5-editor-classic/theme/classiceditor.css
Module parse failed: Unexpected token (6:0)
You may need an appropriate loader to handle this file type.
| */
|
| .ck-editor {
| /* All the elements within `.ck-editor` are positioned relatively to it.
| If any element needs to be positioned with respect to the <body>, etc.,
@ ../node_modules/@ckeditor/ckeditor5-editor-classic/src/classiceditoruiview.js 15:0-36
@ ../node_modules/@ckeditor/ckeditor5-editor-classic/src/classiceditor.js
@ ./src/ckeditor.js
ERROR in ../node_modules/@ckeditor/ckeditor5-ui/theme/components/label/label.css
Module parse failed: Unexpected token (6:0)
You may need an appropriate loader to handle this file type.
| */
|
| .ck-label {
| display: block;
| }
@ ../node_modules/@ckeditor/ckeditor5-ui/src/label/labelview.js 12:0-48
@ ../node_modules/@ckeditor/ckeditor5-ui/src/editorui/boxed/boxededitoruiview.js
@ ../node_modules/@ckeditor/ckeditor5-editor-classic/src/classiceditoruiview.js
@ ../node_modules/@ckeditor/ckeditor5-editor-classic/src/classiceditor.js
@ ./src/ckeditor.js
ERROR in ../node_modules/@ckeditor/ckeditor5-ui/theme/components/tooltip/tooltip.css
Module parse failed: Unexpected token (6:0)
You may need an appropriate loader to handle this file type.
| */
|
| .ck-tooltip,
| .ck-tooltip__text::after {
| position: absolute;
@ ../node_modules/@ckeditor/ckeditor5-ui/src/tooltip/tooltipview.js 12:0-52
@ ../node_modules/@ckeditor/ckeditor5-ui/src/button/buttonview.js
@ ./src/strike.js
@ ./src/ckeditor.js
ERROR in ../node_modules/@ckeditor/ckeditor5-ui/theme/components/icon/icon.css
Module parse failed: Unexpected token (6:12)
You may need an appropriate loader to handle this file type.
| */
|
| svg.ck-icon {
| vertical-align: middle;
| }
@ ../node_modules/@ckeditor/ckeditor5-ui/src/icon/iconview.js 14:0-46
@ ../node_modules/@ckeditor/ckeditor5-ui/src/button/buttonview.js
@ ./src/strike.js
@ ./src/ckeditor.js
npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! notes@3.2.0-dev build-ck: `cd ckeditor-build && webpack && cd ..`
npm ERR! Exit status 2 |
Uh oh!
There was an error while loading. Please reload this page.