-
Notifications
You must be signed in to change notification settings - Fork 84
Review #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Review #13
Conversation
write_csv
List boards
implement slide
…pt with conflicts.
Userboards
flash42
left 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.
Added some comments inline
| users.append(user_data) | ||
| else: | ||
| users = [user_data] | ||
| print(users) |
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.
print in production code?
| }, | ||
| getBoards: function (callback) { | ||
| // the boards are retrieved and then the callback function is called with the boards | ||
| if(this._data.hasOwnProperty('boards')){ |
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.
whitespace 🚔
| let template = document.querySelector('#board_header'); | ||
| let clone = document.importNode(template.content, true); | ||
| let section = document.createElement(`section`); | ||
| section.id = `board${board.id}`; |
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 not a best practice to put ids on everything in the dom. Why do you need it at all?
| 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); |
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.
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. 😕
| // 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'); |
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 use camelCase variable names: templateColumn
| 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'); |
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.
Creating a new card could be extracted to a function, e.g. createCard.
| }); | ||
| }, | ||
| showLoggedIn: function () { | ||
| let username = sessionStorage.getItem("username"); |
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'd suggest using const for variables you don't want to redefine.
| }, | ||
|
|
||
| logout: function () { | ||
| if (sessionStorage.getItem("username")) |
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 don't need to check that getItem gives back a truthy value, you can go ahead and removeItem
|
|
||
|
|
||
| def get_max_id(data): | ||
| print(len(data)) |
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.
printing in production code?
No description provided.