-
Notifications
You must be signed in to change notification settings - Fork 63
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
fix: kurono ui update #1745
fix: kurono ui update #1745
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 1 of 5 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @KamilPawel)
a discussion (no related file):
Please fix capitalisation of buttons + move clear console button to its own bar so the logs don't get written behind it
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 2 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @KamilPawel)
game_frontend/src/containers/IDEEditor/index.js
line 107 at r4 (raw file):
value={this.state.code} width="100%" height="56vh"
problem
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 3 of 5 files at r5, 9 of 10 files at r6, 2 of 2 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @KamilPawel)
a discussion (no related file):
Previously, faucomte97 (Florian Aucomte) wrote…
Please fix capitalisation of buttons + move clear console button to its own bar so the logs don't get written behind it
"Console Log" still needs to be un-capitalised
a discussion (no related file):
The "Exit game" button is missing an icon (cf Miro) https://miro.com/app/board/uXjVOwwbTqY=/
Can we add it in?
game_frontend/src/components/ClearConsoleBar/index.js
line 27 at r8 (raw file):
<StyledConsoleTitle> <ClearButton variant="outlined"
The mockups show the buttons don't have an outline - can you please remove it from this button as well as the "Exit game" button? 🙏
game_frontend/src/components/ClearConsoleBar/index.js
line 29 at r8 (raw file):
variant="outlined" onClick={props.clearConsoleClicked} startIcon={<ClearIcon />}
The icons and the text for all the buttons are not quite aligned, the text isn't quite centered in the button. It would look nicer if the text was centered with the icon.
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 3 of 3 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @KamilPawel)
game_frontend/src/components/ClearConsoleBar/index.js
line 29 at r8 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
The icons and the text for all the buttons are not quite aligned, the text isn't quite centered in the button. It would look nicer if the text was centered with the icon.
Actually they're still not aligned 😞
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 2 of 2 files at r10, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @KamilPawel)
Codecov Report
@@ Coverage Diff @@
## master #1745 +/- ##
===========================================
+ Coverage 0 87.95% +87.95%
===========================================
Files 0 38 +38
Lines 0 1046 +1046
Branches 0 109 +109
===========================================
+ Hits 0 920 +920
- Misses 0 103 +103
- Partials 0 23 +23 |
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 2 of 3 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @KamilPawel)
Description
How Has This Been Tested?
Checklist:
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)