-
Notifications
You must be signed in to change notification settings - Fork 15
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 video info view #1014
Add video info view #1014
Conversation
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.
Thanks, almost perfect!
While reviewing your code I found #1016 so if you feel like it, you could fix that here, too (but you don't have to 😉).
A malformed points array could produce a server error before this change.
Improve video annotation points validation
…oukL-video-info-view
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.
No complaints! Two comments of the previous review are still open.
And don't mind the failing lint check, that's my fault 😉
…oukL-video-info-view
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.
This works great! In addition to the points below I have two comments which are more like my opinion rather than requirements, so please feel free to argue against 😉
First, you show the timestamps in the first row and the respective metadata values in the second row. With lots of values, users have to scroll horizontally. However, users are more used to scrolling vertically. So I would instead show the timestamps in the first column and the values in the second column. This also gives you the chance to add column headers (e.g. "time", "area") in the first row.
Second, you add the metadata values as attributes to the HTML elements. This produces "messy" HTML with lots of escape characters. Example:
v-on:load-modal="getTimes(["2016-12-19T11:27:00.000000Z","2016-12-19T11:28:00.000000Z","2016-12-19T11:29:00.000000Z","2016-12-19T11:30:00.000000Z","2...
In cases like these where I want to pass JS values in the template rather than loading them with an extra API request, I usually do it like this:
@push('scripts')
<script type="text/javascript">
biigle.$declare('videos.metadata', {!! collect($video->metadata) !!});
</script>
@endpush
Then you can use the whole metadata object in volumeMetadata.vue
(or better call it videoMetadata.vue
?) like this:
created() {
this.metadata = biigle.$require('videos.metadata');
}
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.
I've added some final tweaks:
- Increased size of modal to
md
- Fixed flashing of empty table in closing modal
- Updated modal open links to look like actual links
- Removed legacy route that no longer made sense
As stated in Footprint / laser point detection for videos #70, we would like to add a new info view for videos, as for images, with metadata and, later, computed laser points info.