Skip to content
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

Volume slider inside popover on the web interface #150

Merged
merged 2 commits into from May 13, 2020

Conversation

TerryGeng
Copy link
Collaborator

@TerryGeng TerryGeng commented May 12, 2020

This idea has been proposed in #130 long time ago. And popper.js has been included in our repo for a long time.

Now, I have made it true!

In this PR, I also updated popper.min.js to v2.0. That's it. But I didn't check it performance on mobile view. I hope everything works.

An other thing is, it seems that the color of "Filter" section has long became a problem. I fixed it here.

I'd also like to invite @TylerVigario for a review.

Note:

What I have done is following the official tutorials of Popper.js,

I'm confused by document.querySelector("#volume-popover-btn") this line. I tried $("#volume-popover-btn") but it doesn't work. Anyone knows why?

@TerryGeng TerryGeng requested a review from azlux May 12, 2020 11:41
Copy link
Owner

@azlux azlux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good for me.
Look easy to use.
Only one question, is that possible to close the slider if we click out of the slider ?

@TerryGeng
Copy link
Collaborator Author

The design is: if you click that button again, the popover will be closed.

Do you mean that you'd like clicking places outside the popover to be an alternative way of closing it?

@azlux
Copy link
Owner

azlux commented May 12, 2020

yes, It's an habit I have, to close a popup, just click outside instead of looking for the right button.

@TylerVigario
Copy link
Contributor

I don't believe Bootstrap 4 is Popper.js 2.x compatable. We will probably need to remain on 1.x until Bootstrap 5.x.

twbs/bootstrap#29842

@TerryGeng
Copy link
Collaborator Author

@TylerVigario The fact is, I didn't see Popper.js relies on bootstrap... I didn't use the manual from the bootstrap, but instead I directly see the tutorial from popper.js's website. So now I actually treat popper.js as an independent library.

Probably I can rewrite all these code within the framework of bootstrap 4, use the API provided by bootstrap and leave the nuance about popper.js behind. However, what do you think of just invoking popper.js v2 directly until bootstrap add its support for popper.js v2, and I can rewrite things after then?

@TylerVigario
Copy link
Contributor

I'm confused by document.querySelector("#volume-popover-btn") this line. I tried $("#volume-popover-btn") but it doesn't work. Anyone knows why?

You need to get the vanilla object without the jQuery wrapper.

$('#volume-popover-btn').get(0)

@TerryGeng
Copy link
Collaborator Author

... And the reason why I didn't use bootstrap API is I tried several weeks ago when trying to figure out the UI for tagging features, and it turned to be a huge trouble when I try to put <input> into a popover. It seems that the API provided by bootstrap 4 is not friendly if I want to put some html inside it.

@TerryGeng
Copy link
Collaborator Author

You need to get the vanilla object without the jQuery wrapper.

$('#volume-popover-btn').get(0)

Will do later.

@azlux
Copy link
Owner

azlux commented May 12, 2020

@TylerVigario I cannot answer your question, but I juste want to add one information. Bootstrap 5 will not use jquery anymore. Maybe we can try to also not use it. The goal is to use less third-party things, native JS have been improved a lot.

@TylerVigario
Copy link
Contributor

TylerVigario commented May 12, 2020

@TerryGeng I am not too worried about it as we did not use it before. However, there is a hard dependency within Bootstrap 4.x for Popper.js 1.x and could cause issues with components developed with Bootstrap in the future. However, for the moment, it means nothing.

@azlux Sounds good to me. I prefer Vanilla JS for speed and simplicity. However, reusable code is a sign of a good programmer.

I do believe it to be worthwhile to spend some thinking about decoupling the web interface and adding a build flow. I'll open a separate issue for further discussion on the topic.

@TylerVigario
Copy link
Contributor

TylerVigario commented May 12, 2020

Might as well update Bootstrap to v4.5.0 while you are at it.

@TerryGeng
Copy link
Collaborator Author

Yes... I think from now on I will try my best to avoid using jQuery.

@TerryGeng TerryGeng merged commit 4c27bb2 into azlux:master May 13, 2020
@TerryGeng TerryGeng deleted the volume-popover branch May 13, 2020 00:44
@TerryGeng
Copy link
Collaborator Author

@TylerVigario You are right. It seems that the dropdown of bootstrap is implemented by popper and it is not working now.... Let me change things back to popper v1

@TylerVigario
Copy link
Contributor

@terrygang Ahh, yes, the newly added button dropdown, that's right.

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.

None yet

3 participants