Conversation
FritzOnFire
left a comment
There was a problem hiding this comment.
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.
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.
Some formatting comments, but you are done in my opinion :D
| } | ||
| else { |
There was a problem hiding this comment.
Please put this on the same line.
| } | ||
|
|
||
| get to(): number { | ||
| let to = this._from + this.getNumberOfRows() |
There was a problem hiding this comment.
please put a ; at the end of this line.
|
|
||
| for (let row of data) { | ||
|
|
||
| html = html + "<tr>" |
There was a problem hiding this comment.
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.
this can be html += "<td>" + cell + "</td>";
| html = html + "<td>" + cell + "</td>"; | ||
| } | ||
|
|
||
| html = html + "</tr>" |
There was a problem hiding this comment.
this can be html += "</tr>";
| html = html + "<td>" + cell + "</td>"; | ||
| } | ||
|
|
||
| html = html + "</tr>" |
There was a problem hiding this comment.
this can be html += "</tr>";
No description provided.