-
Notifications
You must be signed in to change notification settings - Fork 72
Conversation
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.
Reviewed 9 of 15 files at r1.
Reviewable status: 9 of 15 files reviewed, 1 unresolved discussion (waiting on @mrniket)
game_frontend/src/components/LogEntries/index.js, line 21 at r1 (raw file):
const logEntries = this.props.logs.map(logEntry => <LogEntry key={logEntry.timestamp}
how are we intending timestamp to be calculated? is it sent from the backend? is it epoch milliseconds or similar?
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.
Reviewable status: 9 of 16 files reviewed, 3 unresolved discussions (waiting on @mrniket)
game_frontend/src/components/IDE/index.js, line 12 at r2 (raw file):
logs={[]}
Mind explaining this further? This is a placeholder for props passing?
game_frontend/src/components/IDEConsole/snapshots/index.test.js.snap, line 17 at r2 (raw file):
Object { "log": "bye", "timestamp": 2,
Why is this timestamp 2
and the above one a string? Is there no prop checking?
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.
Reviewable status: 9 of 16 files reviewed, 3 unresolved discussions (waiting on @mrniket)
game_frontend/src/components/IDE/index.js, line 12 at r2 (raw file):
Previously, OlafSzmidt (Olaf Szmidt) wrote…
logs={[]}
Mind explaining this further? This is a placeholder for props passing?
you can look at the Proptypes for IDEConsole :) but a TLDR is that the IDEConsole expects a list of logs
game_frontend/src/components/IDEConsole/snapshots/index.test.js.snap, line 17 at r2 (raw file):
Previously, OlafSzmidt (Olaf Szmidt) wrote…
Why is this timestamp
2
and the above one a string? Is there no prop checking?
ah good point, I'll add it
game_frontend/src/components/LogEntries/index.js, line 21 at r1 (raw file):
Previously, JCobbles (Jacob Moss) wrote…
how are we intending timestamp to be calculated? is it sent from the backend? is it epoch milliseconds or similar?
It will be done/decided in the next task when we connect it to the redux store, for testing I generated it in an epic.
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.
Reviewable status: 9 of 16 files reviewed, 2 unresolved discussions (waiting on @OlafSzmidt)
game_frontend/src/components/IDEConsole/snapshots/index.test.js.snap, line 17 at r2 (raw file):
Previously, mrniket (Niket Shah) wrote…
ah good point, I'll add it
Done.
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.
Reviewed 14 of 15 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
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.
Reviewable status:
complete! all files reviewed, all discussions resolved
Tests broken due to outdated snapshots. I'll take care of them and we can merge |
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.
Reviewed 1 of 1 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
This PR adds the Presentational console component as per UX/UI guidelines. Notable bits of logic:
Also added in this PR is the ability to use class properties
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)