Skip to content
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

Extra change for Moonfire NVR Web UI #315

Merged
merged 13 commits into from
Apr 14, 2024
Merged

Conversation

michioxd
Copy link
Contributor

@michioxd michioxd commented Apr 5, 2024

image

This commit has changed/added those feature below:

  • Fixed lagging on drawer open/close by separating the drawer and header into separate components.

  • Fixed filter layout (used flex).

  • Added dark theme support (including system theme, dark, light).

  • Added outlined filter button if it enabled.

    image

    image

  • Redesign login dialog.

    image

  • Added dual camera layout.

  • Each layout now has a name instead of a class name to display in the layout selector.

  • Added toggle full screen button.

  • Added save current layout and opened cameras to localStorage.

    image

  • Added support for legacy browser (see vite.config.ts)

@michioxd
Copy link
Contributor Author

oh damn I used pnpm instead npm heh...

Copy link
Owner

@scottlamb scottlamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Nice improvements here, and it's a treat for me to see how other people approach UI things since I'm more of a backend developer. Some comments below.

ui/src/components/ThemeMode.tsx Outdated Show resolved Hide resolved
ui/src/components/ThemeMode.tsx Show resolved Hide resolved
display: "flex",
flexDirection: "column",
}}
>
<CardContent>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I guess missing this was why the padding: theme.spacing(1) was necessary before above?

Could you fix the indentation within this element?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need padding for this because the <CardContent /> has done it for us. See full Card usage in the MUI Documentation.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So from the docs, looks like you're totally right that Card should have a CardContent within it.

But I noticed this morning that it spaces out the UI enough to introduce a vertical scroll bar even on my large desktop monitor; Card before vs CardContent after:

image image

  • 12 extra pixels of interior width, I think because one or more of the cards in the column no longer fits quite as well in the column
  • 8+8 => 16+16 pixels of horizontal padding
  • 8+8 => 16+24 pixels of vertical padding
  • margin went away, but I think that just turned into the same space showing up in a different way between elements

Looks like the padding comes from here:

https://github.com/mui/material-ui/blob/9ef12dc81c54566eeceae16bcf026a75fbce504d/packages/mui-material/src/CardContent/CardContent.js#L21-L30

All the extra spacing might be my biggest complaint in general about the mui library, e.g. mui/material-ui#27700.

Looks like I could re-introduce the sx={{ padding: theme.spacing(1) }} on the CardContent and get it closer to before, but I'm not sure how to best undo the last-child bit.

I don't really remember why I picked Card to begin with, though. I just wanted something to group like controls but my usage doesn't really feel like their examples. Should it be just Paper instead? <Paper sx={{ padding: theme.spacing(1) }}> seems to render as before (with the margin going somewhere else I guess):

image

Copy link
Contributor Author

@michioxd michioxd Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think that padding at last-child should separate between CardContent and CardActions (see Material Design 2 Guidelines) but we don't have a plan to use CardActions. Maybe we can override it by adding sx={{padding: 1}} to CardContent

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With sx={{padding: 1}} the last-child thing is still there. iiuc it's more selective so it wins out.

image

12 extra pixels of interior width, I think because one or more of the cards in the column no longer fits quite as well in the column

Huh. By commenting stuff out, I narrowed it down to the "Trim start and end" in the display selector. And it's fine if I add the enclosing FormGroup that it probably should have anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found another solution, just add CardActionArea outside CardContent. Example can be found here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I tried to add override CardContent style by adding some props to extendTheme in index.tsx.

const themeExtended = experimental_extendTheme({
  colorSchemes: {
    dark: {
      palette: {
        primary: {
          main: "#000000",
        },
        secondary: {
          main: "#e65100",
        },
      },
    },
  },
  // this...
  components: {
    MuiCardContent: {
      styleOverrides: {
        root: {
          padding: '16px',
          paddingBottom: '16px !important',
        }
      }
    }
  }
});

Look better now:

image

ui/src/Live/Multiview.tsx Outdated Show resolved Hide resolved
ui/src/Live/Multiview.tsx Outdated Show resolved Hide resolved
ui/src/Live/Multiview.tsx Outdated Show resolved Hide resolved
@michioxd
Copy link
Contributor Author

What I did in this commit:

  • Use your useMediaQuery to dynamically update the theme.
  • I updated some props in ThemeMode and removed non-necessary props.
  • Removed webkitFullScreenElement and msFullScreenElement in Multiview toggle full screen.
  • Added a bit of opacity for the fullscreen button in Multiview.

ui/src/Live/Multiview.tsx Outdated Show resolved Hide resolved
ui/src/components/ThemeMode.tsx Outdated Show resolved Hide resolved
@@ -0,0 +1,6048 @@
lockfileVersion: '6.0'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh damn I used pnpm instead npm heh...

I'm not really up on the JS ecosystem. Is pnpm better enough to be worth switching?

  • If so, we should update everything to match: instructions in guide/build.md, GitHub workflows, removing the package-lock.json
  • otherwise, we should kill this file. It'll be confusing to have both, especially without anything guaranteeing they match.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

release.yml is still using npm, so it's just not locked now and the product might be something different than what was seen in any pre-release testing. We should switch it, too. I think I'd also just have guide/install.md say to use pnpm too because if you do the npm path, you likewise end up with something a little different than what anyone else is running with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, so we will mainly use pnpm right?

@scottlamb
Copy link
Owner

Ci passes! 🎉 just one more comment about the npm->pnpm transition.

@scottlamb scottlamb merged commit 9ede361 into scottlamb:master Apr 14, 2024
7 checks passed
scottlamb added a commit that referenced this pull request Apr 16, 2024
scottlamb added a commit that referenced this pull request Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants