Skip to content

Conversation

@IgorA100
Copy link
Contributor

@IgorA100 IgorA100 commented Jul 29, 2025

Now it looks like this:
1
2

Advantages:

  • Looks nicer and can be flexibly configured.
  • You can move the slider instead of clicking
  • Completely correct operation with Go2RTC and RTSP2Web. I have not tested it with Janus, because it could not work normally for me.
  • Completely synchronized with volume changes (including mute) using the browser's system player.

@IgorA100 IgorA100 marked this pull request as ready for review July 29, 2025 14:46
@IgorA100
Copy link
Contributor Author

@connortechnology
I hope I sent all the changes to this PR
Please check.
I really hope you won't mind this PR.
This slider can be used on any page.
In #4393 I plan to use it on the Montage page.

@IgorA100 IgorA100 marked this pull request as draft July 29, 2025 15:04
@IgorA100 IgorA100 marked this pull request as ready for review July 29, 2025 18:01
@connortechnology
Copy link
Member

How about instead of importing the entire git repo for nouislider, we only import the dist files, in an appropriately named directory (including version number).

It isn't clear to me what this actually gets us and I'm loath to keep including megabytes of js and other stuff just to style a volume input.

After a while people start to complain about bloat. We are already bloated. And keeping track of updates and vulernabilities in these dependencies is what is totally destroying my ability to manage a mobile app. We don't need it in the web ui too.

But that's my bias. I have asked others to review and give their opinion.

@IgorA100
Copy link
Contributor Author

only import the dist files

Yes, of course, I did it + import minified files. Total weight of files loaded into UI = 33kB. I don't think it's a big volume.

It isn't clear to me what this actually gets us

UI, in my opinion, should look beautiful, convenient and functional.
Initially, I planned to use this slider only when using audio visualization #4393 #4338 because it would be necessary in case of disconnecting the video stream and the standard browser player.
3
But after you, Isaac, added a volume control to the Watch page, I decided to also use the slider on the Watch page
My slider works more correctly, it is more pleasant to use.

I'm loath to keep including megabytes of js and other stuff just to style a volume input.

There are no megabytes, there are several kilobytes of useful files.

And keeping track of updates and vulernabilities in these dependencies is what is totally destroying my ability to manage a mobile app

The slider does not use any dependencies and it is written in vanilla JS. I do not like various frameworks and try to use JQuery as little as possible. What mobile app are you talking about? About zmNinja? I do not want to inconvenience you, but I want to have a beautiful, convenient ZM and at the same time not have problems with further code support.

We don't need it in the web ui too.

It's your right, if you don't want to use this slider - you can just close this PR.

I have asked others to review and give their opinion.

Ok, let's wait and see what the others have to say.

@IgorA100
Copy link
Contributor Author

IgorA100 commented Aug 3, 2025

One of the important advantages - Fully synchronizes with volume changes (including mute) using the browser's system player.
That is, when changing the volume level using the browser's system player, the slider value will change and the corresponding cookies will be set.

@IgorA100
Copy link
Contributor Author

IgorA100 commented Aug 6, 2025

@connortechnology
Have you made a decision on this PR?

@connortechnology connortechnology merged commit f047176 into ZoneMinder:master Aug 9, 2025
16 of 19 checks passed
@IgorA100
Copy link
Contributor Author

IgorA100 commented Aug 9, 2025

@connortechnology
I am grateful to you that you finally decided to approve this PR.
I hope this PR will not cause you problems in the future....

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.

2 participants