-
Notifications
You must be signed in to change notification settings - Fork 35
Onboarding Project #34
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?
Conversation
FritzOnFire
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.
Now, there is nothing wrong with having more than one class in the same file, but you have a lot of other logic mixed in with your classes, and app.ts is clearly where you app starts and not where you should be defining classes. Please try and create some more files
jasonkofo
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.
I got a bit tired, especially because I am sure @FritzOnFire is going to cover the other style issues in his PR review. I chose to simply overlook those issues, because it doesn't seem that this code was developed in accordance with the style guide - which is okay until the next sweep of the PR.
A lot of the comments I made are suggestions about the general code design. Please feel free to ask me what I meant if some of the comments that I might give off an air of insanity, or pedantry, because that unfortunately is a thing that often happens.
FritzOnFire
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.
Some formatting comments, but you are done in my opinion :D
| } | ||
| else { |
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 put this on the same line.
| } | ||
|
|
||
| get to(): number { | ||
| let to = this._from + this.getNumberOfRows() |
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 put a ; at the end of this line.
|
|
||
| for (let row of data) { | ||
|
|
||
| html = html + "<tr>" |
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 can be html += "<tr>";
| html = html + "<tr>" | ||
|
|
||
| for (let cell of row) { | ||
| html = html + "<td>" + cell + "</td>"; |
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 can be html += "<td>" + cell + "</td>";
| html = html + "<td>" + cell + "</td>"; | ||
| } | ||
|
|
||
| html = html + "</tr>" |
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 can be html += "</tr>";
| html = html + "<td>" + cell + "</td>"; | ||
| } | ||
|
|
||
| html = html + "</tr>" |
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 can be html += "</tr>";
No description provided.