2 initialize gui #11
2 initialize gui #11
Conversation
There was a problem hiding this comment.
Thanks for working on it! I have a few questions:
Questions
- Is there a specific reason for adding package.json and initializing the react project in the parent directory.
Suggestion
- Please do not push node modules to github. This creates a lot of load, if you have node modules, make sure to add it to gitignore and anyone who wants to run it can use
npm i.
control-station/src/index.tsx
Outdated
There was a problem hiding this comment.
Is there a specific reason index.tsx was created. Since, it is the same as main.tsx. Try to use main.tsx.
control-station/src/App.tsx
Outdated
| import Navbar from './components/Navbar/Navbar'; | ||
| import StatusIndicator from './components/StatusIndicator/StatusIndicator'; | ||
| import ControlPanel from './components/ControlPanel/ControlPanel'; | ||
| import './App.css'; |
There was a problem hiding this comment.
Try to use absolute addresses wherever possible
control-station/src/App.tsx
Outdated
| <Navbar /> | ||
| <StatusIndicator status="green" /> | ||
| <main className="content"> | ||
| {"HX10 GUI"} |
There was a problem hiding this comment.
Since it is just a text and no explicit TS code is being added here, you can just use HX10 GUI instead of {"HX10 GUI"}
| .status-bar { | ||
| position: fixed; | ||
| bottom: 0; | ||
| left: 0; | ||
| right: 0; | ||
| display: flex; | ||
| justify-content: space-around; | ||
| padding: 10px; | ||
| background-color: gray; | ||
| } | ||
|
|
||
| .status-button { | ||
| padding: 10px 20px; | ||
| border: none; | ||
| border-radius: 5px; | ||
| font-size: 1rem; | ||
| font-weight: bold; | ||
| cursor: pointer; | ||
| } |
There was a problem hiding this comment.
Instead of adding padding to status-bar, you can add margin to status-button. This will make the gui more responsive to different screen sizes
| color: white; | ||
| font-weight: normal; | ||
|
|
||
| height: 40px; |
There was a problem hiding this comment.
Any specific reason for adding a constant height to the navbar?
| function Navbar() { | ||
| return ( | ||
| <header className="navbar"> | ||
| <img alt="HX logo" src={HX} style={{ height: "30px" }} /> |
There was a problem hiding this comment.
Use relative units for height of the image, something like rem. This way if I have a different font size, it won't look weird next to the logo
| width: 70px; | ||
| height: 300px; |
There was a problem hiding this comment.
Instead of adding constant height and width to the status indicator, can we make it relative?
| } | ||
|
|
||
| .status-label { | ||
| font-size: 16px; |
There was a problem hiding this comment.
Let's make the font size relative
| width: 40px; | ||
| height: 40px; |
There was a problem hiding this comment.
Can we make this relative to the parent div?
There was a problem hiding this comment.
Is there a reason for having a PNG of the logo too?
preliminary control station setup (intialize Navbar ControlPanel StatusIndicator)