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

[video-toggle] Add align option #894

Merged
merged 4 commits into from
Dec 29, 2022
Merged

[video-toggle] Add align option #894

merged 4 commits into from
Dec 29, 2022

Conversation

MiepHD
Copy link
Contributor

@MiepHD MiepHD commented Nov 22, 2022

Appended video-toggle to #main-panel so it can be centered over the video.
Centered it through setting left to 50% - 120px and set position of #main-panel to relative that it uses this to center.

@@ -18,6 +22,7 @@
color: #fff;
padding-right: 120px;
position: absolute;
left: calc(50% - 120px);
Copy link
Owner

Choose a reason for hiding this comment

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

It works but (opinionated) I preferred having it on the side, it covers less the video 😄 Maybe we can have it as an additional css file, and make it a plugin option to center or not the toggle? This way users have a choice

@MiepHD
Copy link
Contributor Author

MiepHD commented Dec 28, 2022

You can now choose between left, middle and right

@th-ch th-ch merged commit a13606b into th-ch:master Dec 29, 2022
@Araxeus
Copy link
Collaborator

Araxeus commented Jan 9, 2023

This can possibly break the plugin (and it did for me, multiple time)
Cannot read properties of undefined.
mainpanel.style.setProperty("--align", "0px");

this whole code block needs to be inside an apiLoaded closure, we can't guarantee that $('#main-panel') exists when this code is called

In other words, instead of being inside the default export function, it should be inside the setup function which is called only when the page is loaded

@MiepHD @th-ch plz fix

@MiepHD
Copy link
Contributor Author

MiepHD commented Jan 9, 2023

Hm.. that's right. I'll correct that.

@Araxeus Araxeus changed the title Center toggle of video-toggle [video-toggle] Add align option Mar 15, 2023
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.

3 participants