-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implement basic sidebar layout #60
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
4f497f1
to
01d9a41
Compare
3fd0536
to
6b17751
Compare
6b17751
to
267be0c
Compare
267be0c
to
6a0f855
Compare
6a0f855
to
7a7a819
Compare
public/zebrahub-favicon-60x60.png
Outdated
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.
Here are the zebrahub icons if we want a bigger one. We probably also want to set this as our favicon unless we come up with a separate logo for this.
On Chrome, I got an error similar to the following on SO: https://stackoverflow.com/questions/75883720/504-outdated-optimize-dep-while-using-react-vite My simple workaround was to delete |
You got the error running (dev server?) locally I assume? I haven't seen this but will keep an eye out, thanks for documenting it. Any issues with the vercel deployment? |
Yes, it's consistent when switching between this branch and the one associated with #59 |
36af3a6
to
9fe5dd2
Compare
9fe5dd2
to
7ad6f64
Compare
Hm, I can't reproduce but I wonder if it's related to some files that were renamed (capitalized file names) and/or moved. I could especially see this causing issues on macOS since it handles case in file names strangely. |
Ah, could be. My finer grained fix that works is removing |
bde5e8b
to
722f186
Compare
As mentioned above, I don't want to get too carried away in this PR so I'm marking it ready for review/feedback. I think this would be a good spot to stop and demo for the end of the week (can of course build on it if there's time). I guess the final piece here to finish would be to wire up the Cancel/Accept buttons in the Zarr URL popover. The only indication now that the Zarr was not loaded is the red outline on the input field. With this buried in the popover maybe we should put an alert indicator on the canvas. The JSX starts to get much hairier when adding these layout components. I don't know if there's anything to be done about that. |
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.
lgtm!
yeah as you add more layout, it tends to get more complex with how much nesting you need to add to get things aligned right. When the JSX starts to get more deep, it's good to start breaking it down into smaller components for readability for example, we could break down the code in function DrawerHeader() {
return (
<Box
sx={{
flexGrow: 0,
padding: '1em 1.5em',
display: 'flex',
flexDirection: 'row',
alignItems: 'center',
justifyContent: 'space-between',
}}
>
<img src="/zebrahub-favicon-60x60.png" alt="logo" />
<Divider orientation="vertical" flexItem />
<h2>ZEBRAHUB</h2>
</Box>
)
}
function Drawer({ trackControls, dataControls }) {
return (
<Drawer
anchor="left"
variant="permanent"
sx={{
width: drawerWidth,
flexShrink: 0,
'& .MuiDrawer-paper': { width: drawerWidth, boxSizing: 'border-box' },
}}
>
<DrawerHeader />
<Box
sx={{
display: 'flex',
flexDirection: 'column',
justifyContent: 'space-between',
width: '100%',
height: '100%',
}}
>
<Box flexGrow={1} padding="2em">
{trackControls}
</Box>
<Divider />
<Box flexGrow={0} padding="1em">
{dataControls}
</Box>
</Box>
</Drawer>
)
}
function AppContent({ scene, playbackControls }) {
return (
<Box
sx={{
display: 'flex',
flexDirection: 'column',
width: '100%',
height: '100%',
overflow: 'hidden',
}}
>
{scene}
<Box flexGrow={0} padding="1em">
{playbackControls}
</Box>
</Box>
)
}
function App() {
return (
<Box sx={{ display: 'flex', width: '100%', height: '100%' }}>
<AppDrawer
dataControls={
<DataControls
dataUrl={dataUrl}
initialDataUrl={initialViewerState.dataUrl}
setDataUrl={setDataUrl}
copyShareableUrlToClipboard={copyShareableUrlToClipboard}
trackManager={trackManager}
/>
}
trackControls={
<TrackControls
trackManager={trackManager}
trackHighlightLength={trackHighlightLength}
setTrackHighlightLength={setTrackHighlightLength}
clearTracks={() => canvas?.removeAllTracks()}
/>
}
/>
<AppContent
scene={
<Scene
setCanvas={setCanvas}
loading={loading}
initialCameraPosition={initialViewerState.cameraPosition}
initialCameraTarget={initialViewerState.cameraTarget}
/>
}
playbackControls={
<PlaybackControls
enabled={true}
autoRotate={autoRotate}
playing={playing}
curTime={curTime}
numTimes={numTimes}
setAutoRotate={setAutoRotate}
setPlaying={setPlaying}
setCurTime={setCurTime}
/>
}
/>
</Box>
)
} |
Ah yeah, thanks! I think maybe putting the smaller sub-components in the same file improves readability without blowing up the complexity. I hadn't considered that for some reason. |
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.
It's a little hard to read the diff here due to the component refactor and file moves, but the end result looks beautiful to me!
I'm not sure if it's intended or provided in the designs, but one surprising detail is that the bottom layout component (with the time slider) follows my system theme (i.e. dark/light), the but the left layout component is always light.
This wraps the
TrackControls
andDataControls
in aDrawer
so that they appear in a sidebar on the left. I think using theDrawer
component will make it easier to one day show/hide these controls.The rest of the UI is just a vertical flexbox. I'm sure I'm still doing a ton wrong, but this is pretty minimal code and approaches the layout we want. It breaks a bit at very small window sizes, but otherwise works pretty well for me.
A couple other features here - mostly on
DataControls
:Here is a screenshot of this PR:
Compare this to @clarsen-czi's designs in Figma.
I don't want to get too carried away in this PR, so perhaps we should come up with a strategy for sizing design-related PRs. I think once this is merged the remaining changes will be smaller and more orthogonal.
This branch was based on #59, but now rebased on main.