Add 3-2-1 countdown

[denismosolov]

No description provided.

[thijstriemstra]

thanks @denismosolov!

the deviceButton option should go into a separate PR.

In order to make this countdown more flexible I was thinking people would want to configure the:

• time between steps (default: 1000msec)
• amount of steps (default: 3)

The time between steps could be either a nr (1000 is default) or an array of integers.

Thoughts?

[denismosolov]

thanks @denismosolov!

the deviceButton option should go into a separate PR.

In order to make this countdown more flexible I was thinking people would want to configure the:

• time between steps (default: 1000msec)
• amount of steps (default: 3)

The time between steps could be either a nr (1000 is default) or an array of integers.

Thoughts?

Oh, I just forked it to tweak to my own use :) But I can add the parameters to make it customizable later. Also it does not have css yet. Sorry, I mistyped the target repo :)

[denismosolov]

@thijstriemstra thanks for the feedback!

I've updated some docs and got 69 files after running the npm build docs. Should I commit these files?

[thijstriemstra]

Should I commit these files?

No, they'll be updated later.

[denismosolov]

@thijstriemstra Finally, I'd like to lock the record button while the countdown is running. Is that acceptable?

Probably, I have to add a state _contdownActive to the Recorder and move the logic from the RecordToggle#handleClick to the Record#start

[thijstriemstra]

Finally, I'd like to lock the record button while the countdown is running. Is that acceptable?

Makes sense.

[thijstriemstra]

Also, instead of adding two new options, what about adding one that takes an array of msec. This way we only need one option (length of array is amount of steps in the countdown, step length is array element value).

var steps = [1000, 1000, 1000];

or many steps:

var steps = [];
for (var d=0;d<10;d++) {
  steps.push(1000);
}

[denismosolov]

Also, instead of adding two new options, what about adding one that takes an array of msec. This way we only need one option (length of array is amount of steps in the countdown, step length is array element value).

var steps = [1000, 1000, 1000];

or many steps:

var steps = [];
for (var d=0;d<10;d++) {
  steps.push(1000);
}

How about this:

steps: [
{value: 'Three', time: 1000}
{value: 'Two', time: 1000}
{value: 'One', time: 1000}
{value: 'Go!', time: 500}
]

A bit more complicated, but flexible.

[thijstriemstra]

How about this:

Right, makes a lot of sense. I forgot you want to have a custom label for the countdown (or up).

[thijstriemstra]

also needs a changelog entry.

[coveralls]

coverage: 71.822% (+0.7%) from 71.152%
when pulling ab1c7f4 on denismosolov:countdown
into c1af296 on collab-project:master.

[denismosolov]

Not ready yet

[denismosolov]

Hi @thijstriemstra , any luck with the failing tests?

I've just added the final tweaks. Do you think this PR can be merged?

[thijstriemstra]

@denismosolov sorry for the long wait, I would like to get this merged, so could you merge master into your branch?

[denismosolov]

@thijstriemstra merged. I use that countdown for two years in production and it works great. But I need to take a look at everything one more time. I think I'll do it during the April.

Would be great if we can merge it, so I can stop using my fork and switch to the upstream :)

[denismosolov]

@thijstriemstra I wonder if npm run test runs all the tests from the test/ directory on you machine? When I switch to master branch it only runs 38 tests and that number is less that the total amount of tests.

[thijstriemstra]

Yea all tests are run except 1 or so, similar to github actions: https://github.com/collab-project/videojs-record/actions/runs/8589870403/job/23541384429#step:12:2075

What OS/browser versions are you using @denismosolov?

[denismosolov]

Thanks! I'll take a closer look this month.

I can see 106 tests here https://github.com/collab-project/videojs-record/actions/runs/8589870403/job/23541384429#step:12:2075
But, when I run npm run test on my macOS Somona it shows only 36.

[denismosolov]

@thijstriemstra do you think this can be merged? I tested and it looks good to me

[thijstriemstra]

But, when I run npm run test on my macOS Somona it shows only 36.

Do you have firefox and chrome installed?

[thijstriemstra]

i'd rather not add it to the default video demo, to keep the example as simple as possible.

[thijstriemstra]

do you think this can be merged? I tested and it looks good to me

I'll have to test it first, thanks.

[denismosolov]

But, when I run npm run test on my macOS Somona it shows only 36.

Do you have firefox and chrome installed?

Chrome only. I'll install FF and try one more time