[UI] Move Runtime status to BottomAppBar #492
Conversation
apps/ui/src/pages/repo.tsx
Outdated
| @@ -215,14 +216,19 @@ function RepoWrapper({ children, id }) { | |||
| sx={{ | |||
| boxSizing: "border-box", | |||
| width: "100%", | |||
| height: "100%", | |||
| height: "96%", | |||
|
This UI looks fancy! Instead of "moving", I'd like to keep both (essentially two types of "widgets"). We need a "widget" system to allow users to configure what widgets to show in the sidebar and the bottom-status-bar. |
|
I changed the sidebar just a little bit in #493, resulting in a slight conflict. I've merged that PR since it's pretty big and may cause more conflicts. |
|
A second thought: I probably want to hold on this, because the status items/buttons aren’t fixed, and I’m working on adding locally spawned runtimes to these sections. Working with sidebar seems easier to make changes. |
Yeah, I recall this. The only concern is that if we need to maintain 2 sets of code for the same UI system, given that |
SG. Just let me know once you finish the local runtime work. |
|
Thanks @senwang86. My two cents: Can we move the status info to the top bar? The bottom of the screen is usually where the programmer's gaze is at: because code goes top to down. The status bar there sounds a distraction. |
|
We'll have a widget system that allows users to configure "what" widgets to be displayed at "which" location, be it top or bottom. |
|
| return ( | ||
| <ListItem key={runtimeId} disablePadding sx={{ margin: 0 }}> | ||
| {activeRuntime === runtimeId && ( | ||
| <Tooltip title={wsStatus.title}> |
There was a problem hiding this comment.
Crash when deleting a runtime.
There was a problem hiding this comment.
Has it been tested after merging PR #501 ? it works fine on my end though.
There was a problem hiding this comment.
[3/3] And finally, you access wsStatus.title which crashes with "reading properties of undefined" error.
|
I used the bottom bar a little, and I feel that this design is a little over-engineered. The first principle of UI design IMO is to be simple and explicit.
Let's use plain text and refine the UI later. |
Agree. This is also one of the main concerns that I hesitate to merge. I can feel that this will create headaches for future changes. Let's defer the UI polishing to the very end. |
| useEffect(() => {}, [runtimeChanged]); | ||
| const activeRuntime = useStore(store, (state) => state.activeRuntime); | ||
| const setActiveRuntime = useStore(store, (state) => state.setActiveRuntime); | ||
| const runtime = runtimeMap.get(runtimeId) || { wsStatus: "unknown" }; |
There was a problem hiding this comment.
[1/3] you set runtime = {wsStatus: "uknonwn"} as the fallback
| const wsStatusConfig = { | ||
| connected: { title: "connected", color: "green" }, | ||
| connecting: { title: "connecting", color: "yellow" }, | ||
| default: { title: "disconnected", color: "red" }, | ||
| }; | ||
| const wsStatus = wsStatusConfig[runtime.wsStatus || "default"]; |
There was a problem hiding this comment.
[2/3] And if that's unknown, this wsStatusConfig doesn't have the key, which leads to wsStatus===undefined
There was a problem hiding this comment.
Adding some type annotations can help you catch such bugs early and more comprehensively.
| return ( | ||
| <ListItem key={runtimeId} disablePadding sx={{ margin: 0 }}> | ||
| {activeRuntime === runtimeId && ( | ||
| <Tooltip title={wsStatus.title}> |
There was a problem hiding this comment.
[3/3] And finally, you access wsStatus.title which crashes with "reading properties of undefined" error.
Hi @forrestbao , appreciate for the feedback! After some discussion, we agree that the status bar might not offer enough benefit, just as you suggested that it might cause users to be distracted. Thus it will not be merged. |
Summary
A UI refactor which moves the
Sync StatusandRuntime StatusfromSidebarto a newly createdBottomAppBar.Test