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] Midi player measure navigation #952

Open
wants to merge 60 commits into
base: master
Choose a base branch
from

Conversation

haogatyp
Copy link
Collaborator

No description provided.

@sebastian-meyer sebastian-meyer self-requested a review May 24, 2023 19:15
@sebastian-meyer sebastian-meyer changed the title Midi player measure navigation [FEATURE] Midi player measure navigation May 24, 2023
@sebastian-meyer sebastian-meyer added the ⚙ feature A new feature or enhancement. label May 24, 2023
Copy link
Member

@sebastian-meyer sebastian-meyer left a comment

Choose a reason for hiding this comment

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

I did just a rough first review. Before continuing with a detailed code review, please fix those issues:

  • Add .DS_Store files to .gitignore and don't check those files in.
  • Check and fix all the Codacy issues.
  • Resolve the merge conflicts with master branch.
  • Separate genuine third-party Javascript libraries from your own code and/or changed files. As a rule of thumb each third-party library should be put in their own directory (like OpenLayers, jQuery, Toastify, etc.) while changed/own files should be placed in the directory of the Controller which uses the file (like PageView, Search, etc.). The reason is that we exclude third-party libraries from our code quality scans because they usually don't comply with our coding guidelines.

# Conflicts:
#	Classes/Common/MetsDocument.php
#	Classes/Controller/AbstractController.php
#	Classes/Controller/PageViewController.php
#	Configuration/TypoScript/setup.typoscript
#	Resources/Private/Language/de.locallang.xlf
#	Resources/Private/Templates/Navigation/Main.html
#	Resources/Private/Templates/PageView/Main.html
#	Resources/Public/JavaScript/AudioPlayer/mediaplayer.js
#	Resources/Public/JavaScript/PageView/PageView.js
#	Resources/Public/JavaScript/PageView/ScoreControl.js
#	Resources/Public/JavaScript/PageView/ScoreUtility.js
#	Resources/Public/JavaScript/PageView/SyncControl.js
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

# Conflicts:
#	Classes/Common/AbstractDocument.php
#	Classes/Common/IiifManifest.php
#	Classes/Common/MetsDocument.php
#	Classes/Controller/AbstractController.php
#	Classes/Controller/PageViewController.php
#	Classes/Controller/ToolboxController.php
#	Resources/Private/Language/Labels.xml
#	Resources/Private/Templates/Navigation/Main.html
#	ext_conf_template.txt
#	ext_localconf.php
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙ feature A new feature or enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants