Skip to content

Review #13

Open
flash42 wants to merge 36 commits intoCodecoolBase:masterfrom
GaborPap:review-base
Open

Review #13
flash42 wants to merge 36 commits intoCodecoolBase:masterfrom
GaborPap:review-base

Conversation

@flash42
Copy link
Copy Markdown

@flash42 flash42 commented Jun 3, 2019

No description provided.

Copy link
Copy Markdown
Author

@flash42 flash42 left a comment

Choose a reason for hiding this comment

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

Added some comments inline

Comment thread main.py
users.append(user_data)
else:
users = [user_data]
print(users)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

print in production code?

Comment thread static/js/data_handler.js
},
getBoards: function (callback) {
// the boards are retrieved and then the callback function is called with the boards
if(this._data.hasOwnProperty('boards')){
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

whitespace 🚔

Comment thread static/js/dom.js
let template = document.querySelector('#board_header');
let clone = document.importNode(template.content, true);
let section = document.createElement(`section`);
section.id = `board${board.id}`;
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It is not a best practice to put ids on everything in the dom. Why do you need it at all?

Comment thread static/js/dom.js
for (let board of boards) {
if ((board['userid'] === '0' && !board['userid']) || board['userid'] === userid) {
let template = document.querySelector('#board_header');
let clone = document.importNode(template.content, true);
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Clone is a very vague term. Please use something which is more intentional. It is not easy to understand what it really is. You put it in a section, where the section had a board class. 😕

Comment thread static/js/dom.js
// shows the cards of a board
// it adds necessary event listeners also
const board = document.querySelector(`#board${boardId}`);
let template_column = document.querySelector('#board_columns');
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Please use camelCase variable names: templateColumn

Comment thread static/js/dom.js
let clone_columns = document.importNode(template_column.content, true);
clone_columns.querySelector('.board-columns').id = `box${boardId}`;
for (let card of cards) {
let card_template = document.querySelector('#card_sample');
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Creating a new card could be extracted to a function, e.g. createCard.

Comment thread static/js/dom.js
});
},
showLoggedIn: function () {
let username = sessionStorage.getItem("username");
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'd suggest using const for variables you don't want to redefine.

Comment thread static/js/dom.js
},

logout: function () {
if (sessionStorage.getItem("username"))
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You don't need to check that getItem gives back a truthy value, you can go ahead and removeItem

Comment thread util.py


def get_max_id(data):
print(len(data))
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

printing in production code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants