search

scottlamb / moonfire-nvr

Moonfire NVR, a security camera network video recorder
Other
1.2k stars 138 forks source link

ADD react router and reading from query params #193

Closed krystaDev closed 2 years ago

krystaDev commented 2 years ago

Signed-off-by: Damian Krysta damian@krysta.dev

krystaDev commented 2 years ago

If you have some others staff to do on UI, just send me mail :)

scottlamb commented 2 years ago

Thanks! This was a great Saturday surprise! I'll look more closely when I get a chance. I do see the CI failed with this:

src/Live/Multiview.tsx
  Line 12:9:  'isNullOrUndefined' is defined but never used  @typescript-eslint/no-unused-vars
krystaDev commented 2 years ago

I also know other option: Render Routes without cameras, below router (inside ListActiviti or Live) check „isAnyCamerasConfigured”.

This would be „correct way” for me, because routes should be always available, even if cameras wasnt’ fetched - we could render some „loader”.

Damian

Wiadomość napisana przez Scott Lamb @.***> w dniu 02.02.2022, o godz. 07:07:

@scottlamb requested changes on this pull request.

In ui/src/App.tsx https://github.com/scottlamb/moonfire-nvr/pull/193#discussion_r797298103:

@@ -80,6 +85,20 @@ function App() { } };

  • function fetchedCameras(cameras: Camera[] | null) {
  • if (cameras !== null && cameras.length >0) {
  • return (
  • <>
  • <Route path="" element={ <ListActivity cameras={cameras}
  • showSelectors={showListSelectors}
  • timeZoneName={timeZoneName!}
  • />} />
  • <Route path="live" element={} /> There's no matching route on the server to make this work. I did some reading, and I think the options are:

we could modify the server to serve index.html when given this path, but we'll need to update the Rust code when we extend the UI, which might get messy. we could use react-router's HashRouter https://v5.reactrouter.com/web/api/HashRouter. I'm leaning toward the latter; what do you think?

In ui/src/Live/Multiview.tsx https://github.com/scottlamb/moonfire-nvr/pull/193#discussion_r797298544:

const [selected, updateSelected] = useReducer( selectedReducer,

  • Array(MAX_CAMERAS).fill(null)
  • searchParams.has('cams') ? JSON.parse(searchParams.get('cams') || "") : Array(MAX_CAMERAS).fill(null) Do we need the || ""? I still see the behavior change where the list show nothing instead of the (none) it used to, and suspect this is why.

— Reply to this email directly, view it on GitHub https://github.com/scottlamb/moonfire-nvr/pull/193#pullrequestreview-870117490, or unsubscribe https://github.com/notifications/unsubscribe-auth/APA5YO6LFNGRN6LIU4HJIF3UZDC3FANCNFSM5NDFPUEA. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you authored the thread.

scottlamb commented 2 years ago

Render Routes without cameras, below router (inside ListActiviti or Live) check „isAnyCamerasConfigured”.

Sorry, I'm not understanding.

In the options I described:

How does this other option compare?

krystaDev commented 2 years ago

We shouldn’t render routes only if cameras.length > 0 - I just copy this condition. We should we render routes always, and inside routes check cameras.length.

Thats the problem, we got error „no routes find” because api in tests not return any cameras, so any Routes was returned do APP.

Damian Krysta

Wiadomość napisana przez Scott Lamb @.***> w dniu 02.02.2022, o godz. 15:31:

Render Routes without cameras, below router (inside ListActiviti or Live) check „isAnyCamerasConfigured”.

Sorry, I'm not understanding.

In the options I described:

by modifying the server, we'd make the URLs like https://nvr.home.slamb.org/live?cams=%5B0%2Cnull%2Cnull%2Cnull%2Cnull%2Cnull%2Cnull%2Cnull%2Cnull%5D work. Currently the server returns status 404 so the initial load fails. by switching to HashRouter, we'd use URLs like https://nvr.home.slamb.org/#live?cams=%5B0%2Cnull%2Cnull%2Cnull%2Cnull%2Cnull%2Cnull%2Cnull%2Cnull%5D instead, with no changes on the server. How does this other option compare?

— Reply to this email directly, view it on GitHub https://github.com/scottlamb/moonfire-nvr/pull/193#issuecomment-1027997531, or unsubscribe https://github.com/notifications/unsubscribe-auth/APA5YO4W5I3UGDIJFVAD7P3UZE527ANCNFSM5NDFPUEA. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you authored the thread.

krystaDev commented 2 years ago

Aaa.. fuck i dont understand the problem. Lol. 😄

Eyah, we cant change do hash router, but better looks browserRouter than hash .

So, change to hash ?

Wiadomość napisana przez Scott Lamb @.***> w dniu 02.02.2022, o godz. 15:31:

Render Routes without cameras, below router (inside ListActiviti or Live) check „isAnyCamerasConfigured”.

Sorry, I'm not understanding.

In the options I described:

How does this other option compare?

— Reply to this email directly, view it on GitHubhttps://github.com/scottlamb/moonfire-nvr/pull/193#issuecomment-1027997531, or unsubscribehttps://github.com/notifications/unsubscribe-auth/APA5YO4W5I3UGDIJFVAD7P3UZE527ANCNFSM5NDFPUEA. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you authored the thread.Message ID: @.***>

krystaDev commented 2 years ago

Ok. I run prettier, and i FIX "(none)" in selector of cams.

scottlamb commented 2 years ago

Hey, sorry for the long delay. Merging. This is a great UI improvement, and I look forward to building on it to add similar parameters for the list view state (selected cameras, time range, etc.).