Push branch with codebase for code review#1
Conversation
eddubbs
left a comment
eddubbs
left a comment
There was a problem hiding this comment.
I would click (x)Request Changes, but cant request changes over my own code.
I leave it as a comment.
| @@ -0,0 +1,29 @@ | |||
| import './assets/scss/app.scss'; | |||
| import $ from 'cash-dom'; | |||
|
|
|||
| }) | ||
|
|
||
| }) | ||
|
|
|
|
||
| } | ||
|
|
||
| update_profile() { |
There was a problem hiding this comment.
camelCase instead of snake_case. Be consistent with your code
There was a problem hiding this comment.
try to avoid this, it can be misleading,
move this function onto initializeApp() block or make it static
| initializeApp() { | ||
| let self = this; | ||
|
|
||
| $('.load-username').on('click', function (e) { |
There was a problem hiding this comment.
e variable is not used in further implementation, should be ommited
| let self = this; | ||
|
|
||
| $('.load-username').on('click', function (e) { | ||
| let userName = $('.username.input').val(); |
There was a problem hiding this comment.
Username is property of App, so it would be nice to set this property by Object library.
Object.defineProperty(self, "username", {}) and attach getter with validation into it
| @@ -0,0 +1,6 @@ | |||
| import styles from './assets/scss/app.scss'; | |||
There was a problem hiding this comment.
in webpack config you deliver /\.s?css$/,, this is redundant
| const extractPlugin = new ExtractTextPlugin({filename: './assets/css/app.css'}); | ||
|
|
||
| const config = { | ||
|
|
| const config = { | ||
|
|
||
| context: path.resolve(__dirname, 'src'), | ||
|
|
There was a problem hiding this comment.
I'm sceptic about this newline:
If there is ruleset, that after every webpack config key there is a newline, than ommit this, but imho it's redundant.
I'll leave it for discussion, and wont write comment in each key
|
|
||
| module: { | ||
| rules: [ | ||
|
|
| test: /\.(woff|woff2|eot|ttf|otf)$/, | ||
| use: ['file-loader'] | ||
| } | ||
|
|
1 participant
Do code review for this PR