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

feat: add birdseye screen and nav button on home screen #6

Merged
merged 7 commits into from
Sep 20, 2023

Conversation

domneedham
Copy link
Collaborator

Added the Birdseye camera view screen and navigation via a button in the top left of the Home Screen.

Icon should be hidden if the Birdseye config is not enabled and shown if it is. This works for me with the config enabled so hoping @billyjacoby can test since his Birdseye is off.

Copy link
Owner

@billyjacoby billyjacoby left a comment

Choose a reason for hiding this comment

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

overall seems to be working as expected though I don't explicitly have this disabled it still shows the icon but doesn't do anything. The Frigate web UI does the same thing. I'm working on the settings screen now which should allow for changing some of the config values.

Comment on lines 107 to 109
const config = useConfig();

const currentCamera = useAppDataStore(state => state.currentCamera);
Copy link
Owner

Choose a reason for hiding this comment

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

The Header Button should be defined before the actual nav component.

Suggested change
const config = useConfig();
const currentCamera = useAppDataStore(state => state.currentCamera);
const HeaderLeftIcon = ({tintColor}: HeaderBackButtonProps) => {
const nav = useNavigation();
const config = useConfig();
if (!config.data?.birdseye.enabled) {
return null;
}
return (
<BirdseyeIcon
height={25}
width={25}
onPress={() => {
nav.navigate('Birdseye');
}}
fill={tintColor}
fillSecondary={tintColor}
/>
);
};

fillSecondary={tintColor}
/>
);
},
headerTintColor: isDarkMode
Copy link
Owner

Choose a reason for hiding this comment

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

Then we can use is like this in the component.

Suggested change
headerTintColor: isDarkMode
headerLeft: props => HeaderLeftIcon(props),

@@ -98,6 +104,8 @@ const TabNavigator = () => {
};

export const NavigationWrapper = () => {
const config = useConfig();

const currentCamera = useAppDataStore(state => state.currentCamera);
Copy link
Owner

Choose a reason for hiding this comment

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

FYI using tint color like this is currently causing a warning since it's sort of an HSL value, but I don't think RN knows how to parse that. I'll be addressing this soon when I try and overhaul theming a bit.

@billyjacoby
Copy link
Owner

overall seems to be working as expected - doesn't show me the icon without birdseye enabled.

@domneedham
Copy link
Collaborator Author

Awesome, I’ll tidy up the PR and then can merge. Thanks for testing it out

@domneedham
Copy link
Collaborator Author

@billyjacoby PR I think is ready if you just want to confirm 👍

@billyjacoby
Copy link
Owner

So this doesn't actually connect to BirdsEye for me. Looking at my birdseye network tab it looks like the video connection is pretty different compared to the WebRTC camera streams, for me Birdseye is a JSMpeg video.

Also the icon looks a bit wonky for me:
Screenshot 2023-09-20 at 6 53 32 AM

I think we should use the default background bird image for this icon.

@domneedham
Copy link
Collaborator Author

Hmm, do you have restream enabled for Birdseye? I wonder if that makes it so the go2rtc powers the webrtc part of it. I think that was added in 0.12.

No problem with changing the icon, can you clarify which one you mean though? Was looking for an svg that suits ...

@billyjacoby
Copy link
Owner

Hmm, do you have restream enabled for Birdseye? I wonder if that makes it so the go2rtc powers the webrtc part of it. I think that was added in 0.12.

No problem with changing the icon, can you clarify which one you mean though? Was looking for an svg that suits ...

Icon:
Screenshot 2023-09-20 at 6 59 00 AM

I can turn it into an SVG in Figma if there isn't one readily available, just lmk.

Updating the restream value now and checking again.

Sidenote, if we could add jsmpeg support that would reduce our need for WebRTC and hopefully work over VPN.

@billyjacoby
Copy link
Owner

Yeah adding restream to my config fixed it. lgtm!

billyjacoby
billyjacoby previously approved these changes Sep 20, 2023
@domneedham
Copy link
Collaborator Author

If you could make it an svg that would be great, don't think I'd be able to do that.

Okay, will add a check too for restream being enabled and show an appropriate error if it's not.

@domneedham
Copy link
Collaborator Author

Updated Birdseye screen so it won't try to show webrtc if restream is off. Waiting on icon and then ready to merge I think 👍

@billyjacoby
Copy link
Owner

frigate-bird
I think this should be good for the icon, but double check it since it isn't a square.

@domneedham
Copy link
Collaborator Author

Perfect yeah, had to add a little margin on the top to make it aligned with other icons but that's fine.

component={BirdseyeScreen}
options={{
headerBackTitle: 'Home',
}}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
}}
presentation: 'modal',

Last nit - I think this should actually be presented in a Modal since the button to access is in the nav header. It feels a bit weird that it slides in as a card now. I'm okay with adding this later too, but wanted to call it out.

@domneedham domneedham merged commit cd8c045 into billyjacoby:main Sep 20, 2023
2 checks passed
@domneedham
Copy link
Collaborator Author

Yeah can take a look at doing that down the line. Not sure if it makes sense with it being a camera page, but then not sure what else would ever get added to that screen anyway, as it's not like events or anything can live there.

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