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

WIP pull request - ERC viewable as galley #1

Merged
merged 15 commits into from
Jul 9, 2021

Conversation

tomniers
Copy link
Collaborator

@tomniers tomniers commented Nov 1, 2019

@nuest
Here my first steps with the plugin!

@tomniers tomniers changed the title first try DOI display in console WIP pull request - ERC viewable as galley Jun 4, 2021
@tomniers
Copy link
Collaborator Author

tomniers commented Jun 4, 2021

@nuest @Fmazin
Here a first WIP pull request. It would be great if you @Fmazin take a look at whether everything is understandable and works as presented last week and if necessary comment or accept.

Copy link
Member

@nuest nuest left a comment

Choose a reason for hiding this comment

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

Are the ...chunk.css/js/map files not part of the UI download? Do they need to be saved in the plugin?

Regarding my other comments: I'm happy to merge without them being handled, we can create issues instead.

build/config.js Show resolved Hide resolved
@@ -0,0 +1,15 @@
{
"short_name": "React App",
Copy link
Member

Choose a reason for hiding this comment

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

Change short name and name to something more reasonable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@NJaku01 Can you take care of it so that it is done in general for the build?

Copy link

@njakuschona njakuschona Jun 25, 2021

Choose a reason for hiding this comment

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

I have adapted it in my fork, the changes will be available in the build with the next release.

$manifestNew = $baseUrl . '/' . $this->getPluginPath() . '/build' . substr($manifestOld[0], 1);
$adaptedManifest = str_replace($manifestOld[0], $manifestNew, $adaptedLogo);

preg_match('"\.\/config\.js"', $readHtmlFile, $configOld);
Copy link
Member

Choose a reason for hiding this comment

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

Can you encapsulate this in a function? Looks very repetitive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah is done here: a23c335

@@ -0,0 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE version SYSTEM "../../../lib/pkp/dtd/pluginVersion.dtd">
Copy link
Member

Choose a reason for hiding this comment

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

This path does not exist in this repository - I assume that is not a problem? Or exchange the dtd URL with a correct online URL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a default of the ojs repository. In the OJS repository the path leads to the plugin version. So I think its correct.

version.xml Outdated
<version>
<application>ojs-erc-plugin</application>
<type>plugins.generic</type>
<release>0.1.0.0</release>
Copy link
Member

Choose a reason for hiding this comment

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

Are 4 digits required for the version, or can we also stick to major.minor.bugfix ?

Copy link
Collaborator Author

@tomniers tomniers Jun 25, 2021

Choose a reason for hiding this comment

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

No we can also stick to 3 digits, the corresponding commit can be found here: eaea77a

version.xml Outdated
<application>ojs-erc-plugin</application>
<type>plugins.generic</type>
<release>0.1.0.0</release>
<date>2019-05-15</date>
Copy link
Member

Choose a reason for hiding this comment

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

Please start the notes for creating releases and document how this file needs to be updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The notes are now available via a description in the README.md and the CHANGELOG.md. The corresponding commit can be found here: eaea77a

@tomniers
Copy link
Collaborator Author

Are the ...chunk.css/js/map files not part of the UI download? Do they need to be saved in the plugin?

Regarding my other comments: I'm happy to merge without them being handled, we can create issues instead.

Concerning your question, the ...chunk.css/js/map files are part of the UI download and thus they are downloaded within the build folder. Of course the build folder does not have to be part of this repository, because it is downloaded automatically when the plugin is activated.
All comments have been edited appropriately or converted to issues and can be resolved if agreed.

@nuest nuest merged commit 0d90f28 into o2r-project:master Jul 9, 2021
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