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

Feature/context menu #33

Merged
merged 1 commit into from
Dec 21, 2016
Merged

Feature/context menu #33

merged 1 commit into from
Dec 21, 2016

Conversation

OPersian
Copy link

No description provided.

@OPersian OPersian requested a review from z4y4ts December 21, 2016 10:58
@OPersian OPersian assigned z4y4ts and unassigned z4y4ts Dec 21, 2016
Copy link

@z4y4ts z4y4ts left a comment

Choose a reason for hiding this comment

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

Looks good in general.

Can you get rid of jQuery though? May be in the next Pull Request.

.vjs-contextmenu-ui-submenu .vjs-submenu-item:hover {
background-color: rgba(0, 0, 0, 0.5);
text-shadow: 0em 0em 1em white;
}
Copy link

Choose a reason for hiding this comment

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

Please add new line

var player = videojs('{{ video_player_id }}');

// Delegate creation of a nested submenu for a context menu
$(videoPlayer).on('mouseenter', 'li.vjs-menu-item:last', function(e) {
Copy link

Choose a reason for hiding this comment

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

Why don't you use player.on() event binding function?
Event name may be different, but I don't see reason we need jQuery here.

See code samples here http://blog.videojs.com/Hiding-and-Showing-Video-Player-Controls/

Copy link
Author

Choose a reason for hiding this comment

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

Event delegation seems not to work correctly if an event is bound to a videojs object. Would prefer to re-write with pure JavaScript in the next PR.

Copy link

Choose a reason for hiding this comment

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

Ok, then please do so.

I approve this PR for the sake of demo, but please continue with refactoring this before switch to another task.

@@ -0,0 +1,90 @@
/**
Copy link

Choose a reason for hiding this comment

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

I know our js file names are a bit messy, but we need to stick to something. Let it be dashes rather than underscores.

So can you please rename it to player-context-menu.js

Copy link

@z4y4ts z4y4ts left a comment

Choose a reason for hiding this comment

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

Squash and merge.

Please be sure to write good commit message.

Add context menu

Created context menu with videojs-contextmenu-ui plugin
Created a nested menu for a context menu

Fix nested submenu creation sequence

Prevented nested submenu duplicates
Set an event handler to a context menu item to create a nested submenu

Enable custom context menu for each backend separately

Minor changes as per convention
@OPersian OPersian force-pushed the feature/context-menu branch from 3393b33 to 212f34a Compare December 21, 2016 16:12
@OPersian OPersian merged commit 5e7a5cb into dev Dec 21, 2016
dyudyunov pushed a commit that referenced this pull request Jan 13, 2022
Add Vanta required PR and Issue templates
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