-
Notifications
You must be signed in to change notification settings - Fork 12
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
#1392: Generate still images for video, choose modal #1393
Conversation
and to add outline to selection. Works on Sarari but includes the radio bullet needed by Safari.
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.
Hey, I'd like to test this functionally, but from the information I see in the pull request or the issues, I don't understand where exaclty on signbank-susan
I can find this... can you explain this to me in the form of a screenshot?
Already a technical remark: I'm confused by the function naming here. I see gloss_video_stills_ajax
, create_stills
... another create_stills
, make_thumbnails
. I think more descriptive names would make this easier to understand what is the difference between all of these.
I copied the naming that was used in similar functionality urls. I prefer the name stills but other functions also use citation image or thumbnail. There are already many similar functions and methods so it's difficult to identify them from each other. Unless we go for really long phrases with underscores? Is that better for being clear? Or worse for being long-winded? I will try to rename them, but it could end up being worse. |
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.
Just had a look, I think the function names are way better now, thanks! I was also able to find the button to test it functionally now (again, thanks :) ).
One remark: there are separate buttons now for 'generate stills' and 'show stills', but I think this is too low level for the end user... the end user just wants to choose still. I propose to always do 'generate' and 'show' each time this modal is opened, and hide the buttons for the user.
The changes were made yesterday as requested. The code works fine. No timeout necessary. Since you have limited time to review, I would prefer that you spend that time reviewing the "reversion bug fix" pull request. |
The code shows the radio bullet and outlines the chosen image in red inside a modal pop-up.
(Radio bullets are shown in the display of still images. On Firefox, it works to hide the input bullet, but that does not work on Safari, so the bullets are visible.)
A frame rate of 15fps is used to extract the images. The first second is ignored from the video as per feature request.
It may be necessary to show more images than 15 fps. But that would be an awful lot of images. See how it goes in usage.