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

Add screenshot button to video annotator #1004

Merged
merged 26 commits into from
Jan 28, 2025
Merged

Add screenshot button to video annotator #1004

merged 26 commits into from
Jan 28, 2025

Conversation

lehecht
Copy link
Contributor

@lehecht lehecht commented Dec 6, 2024

Closes #956.

@lehecht lehecht changed the title Add screenshot button for videos Add screenshot button to video annotator Dec 9, 2024
@lehecht
Copy link
Contributor Author

lehecht commented Dec 9, 2024

@mzur
How can I check if my button is disabled for non-CORS enabled videos? Do you know any websites with non-CORS enabled videos that I could use to test this? Or did you use a different approach for testing?

@mzur
Copy link
Member

mzur commented Dec 9, 2024

On your local instance you could use the BIIGLE landing page video.

@lehecht lehecht marked this pull request as ready for review December 12, 2024 08:06
@lehecht lehecht requested a review from mzur December 12, 2024 08:07
Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

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

I think I prefer not to use provide/inject here as it hides where the data is coming from. Maybe this is a chance to refactor the screenshot button a bit so it gets all data it needs through props. E.g. if it gets the image/video ID as a prop, it does not even need the image/video changed event any more.

The map.init event must stay, though, as the map object must not be passed as a prop. It should never be made reactive.

resources/assets/js/videos/videoContainer.vue Outdated Show resolved Hide resolved
resources/assets/js/annotations/annotatorContainer.vue Outdated Show resolved Hide resolved
@lehecht
Copy link
Contributor Author

lehecht commented Dec 13, 2024

@mzur

Maybe this is a chance to refactor the screenshot button a bit so it gets all data it needs through props

I also thought about it, but then I would need to refactor the video and image settingsTab, too. The settingsTabs import the screenshotButton, which would then need the same properties as the screenshotbutton. I am not sure, if they should get props that they will not use except to pass them to the screenshotbutton.

@mzur
Copy link
Member

mzur commented Dec 18, 2024

If it's not too much, I think it is fine to pass the props through the settings tab.

Alternatively you could provide the individual properties (e.g. current ID, current filename) instead of a single "files" object. This is at least more descriptive and you can find the location where these are coming from easier than with the generic name. This would still be better than the solution with the event.

@lehecht lehecht requested a review from mzur January 8, 2025 09:02
resources/assets/js/annotations/annotatorContainer.vue Outdated Show resolved Hide resolved
resources/views/annotations/show.blade.php Outdated Show resolved Hide resolved
resources/views/videos/show.blade.php Outdated Show resolved Hide resolved
resources/assets/js/videos/videoContainer.vue Outdated Show resolved Hide resolved
@lehecht lehecht requested a review from mzur January 27, 2025 09:55
Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

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

Thanks!

@mzur mzur merged commit dfa70da into master Jan 28, 2025
6 checks passed
@mzur mzur deleted the screenshot-videos branch January 28, 2025 11:01
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.

Screenshot button for videos
2 participants