search

publiclab / infragram

A minimal core of the Infragram.org project in JavaScript
https://infragram.org/sandbox/
GNU General Public License v2.0
45 stars 166 forks source link

Canvas Recording and Video resolution change #446

Closed Forchapeatl closed 2 years ago

Forchapeatl commented 2 years ago

Canvas Recording and Video resolution change Fixes #418 fount at https://forchapeatl.github.io/infragram/indexs.html Passed Test Case

Failed Test Case

https://user-images.githubusercontent.com/24577149/184511540-2cf9e25f-1d56-4b28-9a13-e73c4d575c08.mp4

Make

sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

gitpod-io[bot] commented 2 years ago

Forchapeatl commented 2 years ago

@jywarren , @TildaDares and @stephaniequintana may I please have your reviews on this PR ?

jywarren commented 2 years ago

Love the color change idea. For accessibility, it's often a good idea to change the icon of the button too, or to invert the colors, so the icon is dark and the background light (or reverse depending). That way it still works for folks who don't see colors the same way. You could make a CSS class which overrides the styles and toggle that class in jQuery, or you could swap a FontAwesome icon by changing the classname.

On Mon, Aug 15, 2022 at 2:25 PM Stephanie Quintana @.***> wrote:

@.**** commented on this pull request.

In src/ui/interface.js https://github.com/publiclab/infragram/pull/446#discussion_r946012008:

  • $('#startRecord').click(function(e){

  • const chunks = [];

  • rec.ondataavailable = e => chunks.push(e.data);

  • rec.onstop = e => exportVid(new Blob(chunks, {type: 'video/h264'}));

  • rec.start();

  • document.getElementById('startRecord').style.display='none';

  • document.getElementById('stopRecord').style.display='block';

  • })

  • $('#stopRecord').click(function(e){

  • rec.stop();

  • document.getElementById('stopRecord').style.display='none';

  • document.getElementById('startRecord').style.display='block';

  • document.getElementById('downloadButton').style.display='block';

  • })

As a whole, it's functioning well when I test it. My only suggestion would be to consider changing the background color of the record button to red while recording. Perhaps in addition to toggling the display from none/block we can add that here. Great job, @Forchapeatl https://github.com/Forchapeatl 🚀

— Reply to this email directly, view it on GitHub https://github.com/publiclab/infragram/pull/446#pullrequestreview-1073032940, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAF6J4A6QKV3XBEUOTA5QDVZKDQNANCNFSM56NZK7CQ . You are receiving this because you were mentioned.Message ID: @.***>

Forchapeatl commented 2 years ago

@jywarren this PR is ready to merge. May I ?

jywarren commented 2 years ago

Hi @Forchapeatl I had not yet approved this one because i wanted to know your views on the questions above - can you reply up there? I'd like to hear from you on each of them, and had hoped for the function changeResolution to be changed in this PR. No worries, these weren't critical changes, but please do your best to respond to questions and clarifications before merging, thank you!

Forchapeatl commented 2 years ago

Hi @Forchapeatl I had not yet approved this one because i wanted to know your views on the questions above - can you reply up there? I'd like to hear from you on each of them, and had hoped for the function changeResolution to be changed in this PR. No worries, these weren't critical changes, but please do your best to respond to questions and clarifications before merging, thank you!

Sorry about that @jywarren , I didn't realize my poor actions. For this behavior I am very sorry. It wont happen again.

jywarren commented 2 years ago

No worries! Let me know what your responses are to the questions!

On Fri, Aug 19, 2022, 8:47 PM FORCHA @.***> wrote:

Hi @Forchapeatl https://github.com/Forchapeatl I had not yet approved this one because i wanted to know your views on the questions above - can you reply up there? I'd like to hear from you on each of them, and had hoped for the function changeResolution to be changed in this PR. No worries, these weren't critical changes, but please do your best to respond to questions and clarifications before merging, thank you!

Sorry about that @jywarren https://github.com/jywarren

— Reply to this email directly, view it on GitHub https://github.com/publiclab/infragram/pull/446#issuecomment-1221191557, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAF6J33FNYYK2UYMKDJF4DV2ATJVANCNFSM56NZK7CQ . You are receiving this because you were mentioned.Message ID: @.***>