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

Support for musical sources (and prototype annotations) #230

Merged
merged 59 commits into from
Jul 22, 2024

Conversation

haogatyp
Copy link
Contributor

No description provided.

Resources/Private/Less/modules/score.less Outdated Show resolved Hide resolved
@sebastian-meyer
Copy link
Member

@haogatyp Please resolve the merge conflicts.

@sebastian-meyer sebastian-meyer marked this pull request as draft September 27, 2023 09:03
@haogatyp haogatyp marked this pull request as ready for review October 17, 2023 09:00
@beatrycze-volk beatrycze-volk added the ✔️ feature A new feature or enhancement. label Oct 18, 2023
chrizzor added 3 commits June 11, 2024 17:01
# Conflicts:
#	Build/package-lock.json
#	Classes/Controller/UriController.php
#	Resources/Private/JavaScript/dfgviewerScripts.js
#	Resources/Private/Less/all.less
#	Resources/Private/Less/components/audioplayer.less
#	Resources/Private/Less/components/controls.less
#	Resources/Private/Less/modules/home.less
#	Resources/Private/Partials/ControlBar.html
#	Resources/Private/Partials/PageView.html
#	Resources/Public/Css/allStyles.css
#	Resources/Public/Css/webStyles.css
#	Resources/Public/JavaScript/allScripts.js
#	Resources/Public/JavaScript/webScripts.js
Resources/Private/JavaScript/dfgviewerScripts.js Dismissed Show dismissed Hide dismissed

// toggle for full metadata display in sidebar
if ($('.control-bar .metadata-basic dl.tx-dlf-metadata-titledata').length > 1) {
metadataToggleLabelMore = ($('html[lang^="de"]')[0]) ? 'mehr Metadaten anzeigen' : 'more Metadata';

Check warning

Code scanning / CodeQL

Missing variable declaration Warning

Variable metadataToggleLabelMore is used like a local variable, but is missing a declaration.
// toggle for full metadata display in sidebar
if ($('.control-bar .metadata-basic dl.tx-dlf-metadata-titledata').length > 1) {
metadataToggleLabelMore = ($('html[lang^="de"]')[0]) ? 'mehr Metadaten anzeigen' : 'more Metadata';
metadataToggleLabelLess = ($('html[lang^="de"]')[0]) ? 'weniger Metadaten anzeigen' : 'less Metadata';

Check warning

Code scanning / CodeQL

Missing variable declaration Warning

Variable metadataToggleLabelLess is used like a local variable, but is missing a declaration.
Resources/Private/JavaScript/dfgviewerScripts.js Outdated Show resolved Hide resolved
Resources/Private/Less/all.less Outdated Show resolved Hide resolved
@sebastian-meyer sebastian-meyer changed the title Full text features main Support for musical sources (and prototype annotations) Jul 8, 2024
Co-authored-by: Sebastian Meyer <sebastian.meyer@opencultureconsulting.com>
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.

Last fixes for the remaining static code analyzer issues. Also, please have a look at the Codacy issues: https://app.codacy.com/gh/slub/dfg-viewer/pull-requests/230/issues

Resources/Private/JavaScript/dfgviewerScripts.js Outdated Show resolved Hide resolved
Resources/Private/JavaScript/dfgviewerScripts.js Outdated Show resolved Hide resolved
chrizzor and others added 3 commits July 10, 2024 17:40
Co-authored-by: Sebastian Meyer <sebastian.meyer@opencultureconsulting.com>
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.

Good work! This is ready for testing, now!

The only remaining issue is with the new Grunt version. Could you please check that?
(see https://github.com/slub/dfg-viewer/actions/runs/9877302151/job/27494458932?pr=230)

@chrizzor
Copy link
Contributor

It looks like Thomas Jung @gnuj has set the version of grunt in the package.json to ^1.6. The installed version therefore does not match the specification in the package-lock.json file. Perhaps Thomas Jung still needs to commit the lock file? I'm not sure.

@sebastian-meyer
Copy link
Member

@beatrycze-volk From my point of view, this is ready for merging. The corresponding PR kitodo/kitodo-presentation#1281 has been merged.

This can be tested at https://test.dfg-viewer.de

The remaining Grunt issue should be easily fixed by running npm update once. Unfortunately I can't do that because I don't have access to the source branch.

@gnuj
Copy link
Contributor

gnuj commented Jul 22, 2024

The remaining Grunt issue should be easily fixed by running npm update once. Unfortunately I can't do that because I don't have access to the source branch.

Thats completely correct. This will probably be significantly less work than if I start a complete new branch including a merge request. If this is important for organisational reasons just hit me up.

@sebastian-meyer
Copy link
Member

Since we all can't access the source branch (and @chrizzor isn't available for the next two months), I propose merging this, then immediately run npm update and push the changes again. Is that OK, @beatrycze-volk?

@beatrycze-volk
Copy link
Contributor

Hi @sebastian-meyer, I currently have no access to my computer so please feel free to make it :)

@sebastian-meyer
Copy link
Member

OK, you are the release manager, I'll do as you say! ;o)

@sebastian-meyer sebastian-meyer merged commit 7261790 into slub:master Jul 22, 2024
5 of 6 checks passed
@stweil
Copy link
Contributor

stweil commented Jul 22, 2024

I'm afraid that Resources/Public/Css/allStyles.css.map should not have been added. Pull request #294 removes it again.

@stweil
Copy link
Contributor

stweil commented Jul 22, 2024

For future contributions it would be good to set a valid author name.

Copy link
Contributor

@beatrycze-volk beatrycze-volk left a comment

Choose a reason for hiding this comment

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

@sebastian-meyer Unfortunately this PR has broken few functionalities inside the DFG Viewer. eg. the calendar view for the newspapers is gone, the fulltext window is gone. I have to revert those changes.

I have left few review comments, but they contain only the problems discovered on the first look. After they are fixed and the removed functionalities are restored, I will make more through review.

@@ -8,7 +8,7 @@ config {
removeDefaultJS = 1

# concatenate all Javascript files by default
concatenateJs = 1
concatenateJs = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be changed.

@@ -35,6 +36,11 @@ page {
includeJSFooterlibs {
dfgviewer = EXT:dfgviewer/Resources/Public/JavaScript/allScripts.js
}
footerData {
# Add OpenLayers styling for the scores (This is 'dirty' integrated via footerData to ensure that it overrides the styles brought by DLF and other extensions)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's not the right solution.

@@ -57,6 +57,34 @@
<source><![CDATA[Thumbnail Preview]]></source>
<target><![CDATA[Vorschaubilder]]></target>
</trans-unit>
<trans-unit id="multiview.add_document.description">
Copy link
Contributor

Choose a reason for hiding this comment

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

camelCase should be used in he names to keep consistency with the rest of the labels.

@@ -13,6 +13,14 @@
bottom: 0;
background: #fff;
text-align: left;

display: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

Fulltext can't be hidden. It breaks one of the main functionalities of DFG Viewer!

@@ -0,0 +1,21 @@
<!-- ###TEMPLATE### begin -->
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not even sure what has happend here...

<div class="score-tool">
<f:if condition="{scoreAvailable.{iterator.index}}">
<f:then>
<a class="select switchoff" id="tx-dlf-tools-score-{iterator.index}" title="" data-dic="fulltext:{f:translate(key: 'tools.score')}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this fulltext or score tool?

</f:else>
</f:if>
<f:comment>
<f:cObject typoscriptObjectPath="plugin.tx_dlf_scoretool" />
Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldn't be commented code introduced.

<div id="svg_output" style="overflow:hidden;"></div>
</div>

<f:comment>
Copy link
Contributor

Choose a reason for hiding this comment

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

As previously mentioned no commented code.

@@ -3,7 +3,7 @@
<div xmlns="http://www.w3.org/1999/xhtml" lang="en" xmlns:f="http://typo3.org/ns/TYPO3/CMS/Fluid/ViewHelpers">

<f:section name="Content">
<f:format.raw>{content}</f:format.raw>
<f:format.raw>{ccontent}</f:format.raw>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the name is changed?

/**
* @return {ol.style.Style}
*/
dlfViewerOLStyles.hoverStyle = function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are open layer not included in kitodo presentation? Why additionally here?

@sebastian-meyer
Copy link
Member

@chrizzor has to take a look at this and fix these issues, since it's his work. But he won't be back before end of september.

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.

8 participants