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

Refactor contextmenu creation #41

Merged
merged 2 commits into from
Jan 3, 2017
Merged

Refactor contextmenu creation #41

merged 2 commits into from
Jan 3, 2017

Conversation

OPersian
Copy link

Refactored menu labels update
Changed playback rate assignment
Moved hide/show of submenu to css

@OPersian OPersian requested a review from z4y4ts December 27, 2016 13:18
@z4y4ts z4y4ts requested a review from polesye December 27, 2016 14:51
@z4y4ts
Copy link

z4y4ts commented Dec 27, 2016

@polesye please take a look. You've had some concerns regarding context menu implementation IIRC.

@@ -16,82 +19,93 @@ domReady(function() {
// VideoJS Player() object necessary for context menu creation
var player = videojs('{{ video_player_id }}');

/**
* Create elements of nested context submenu.
*/
function createNestedContextSubMenu(e) {
var target = e.target;

if (target.matches("li.vjs-menu-item")
&& target.innerText == player.contextmenuUI.content[3].label
Copy link

Choose a reason for hiding this comment

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

Can we fix content[3] here?

liSubMenu.onclick = function() {
player.playbackRate(parseFloat(playbackRate));
}
})();
Copy link

@polesye polesye Dec 27, 2016

Choose a reason for hiding this comment

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

it's better to use DocumentFragment in bulk DOM element generations.
forEach method can also help you to simply the code.

Copy link

Choose a reason for hiding this comment

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

Copy link

@polesye polesye left a comment

Choose a reason for hiding this comment

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

Nice job!

Add context menu
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

Refactor context menu

Refactor with pure JavaScript

Refactor hiding a nested submenu

Refactor contextmenu creation

Refactored menu labels update
Changed playback rate assignment
Moved hide/show of submenu to css

Optimized submenu creation
@OPersian OPersian force-pushed the feature/context-menu branch from b078f48 to 31218fc Compare January 3, 2017 15:26
@OPersian OPersian merged commit 7d67dc6 into dev Jan 3, 2017
z4y4ts added a commit that referenced this pull request Jan 6, 2017
…linter

* commit '324aa43674d214d184637bf3ec52331495fe12da':
  Enable popup submenu in Edge
  Fixed Speed label for Brightcove
  Remove unused static assets
  Disable .srt uploads for transcripts as this format not supported by VideoJS
  Add language select
  Not include videojs-transcripts.js to page if no transcripts (#50)
  Refactor of including javascript content to brightcove player
  Add caption for video
  Enabling/disabling of interactive transcripts
  Optimized submenu creation (#41)
  Add download transcript button for students
  Buttons for videojs player
  Replace videojs playback rate control with custom
  Refactor context menu
  Feature/player id validation (#37)
  Save state on next button clicking (#39)
  Add text "Speed" on large viewports
  Refactor context menu
@z4y4ts z4y4ts deleted the feature/context-menu branch February 9, 2017 13:50
dyudyunov pushed a commit that referenced this pull request Jan 13, 2022
…unity-fixes

1.0.0 community alpha ... merge (broken) community fork changes toward Py3 / Juniper compat
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