-
Notifications
You must be signed in to change notification settings - Fork 47
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
[UI-LIBRARY][main] #7505 Initial Commit Upload - Delete - Download JATS File #300
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll send this along to someone else for a proper review when we're ready, but in the meantime, just a quick comment!
@jardakotesovec, would you mind taking a look at this PR (part of pkp/pkp-lib#7505)? Thanks! |
@defstat, I think you were going to raise something specific here about error handling? |
@jardakotesovec I was having some trouble figuring out how I should handle possible errors. Is there a "standard" way of doing that when using PKP's vue.js components? Are we handling exceptions in a particular way? |
.filePanel__header { | ||
grid-column: span 12; | ||
background: #f3f3f3; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class does not seems to exist, maybe just something to clean up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does exist though here.
Are you perhaps referring to something else?
@@ -0,0 +1,392 @@ | |||
<template> | |||
<div class="jatsPanel"> | |||
<slot> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better not to categorize this components as ListPanel. Not on PHP side nor Vue.js side. Its very custom component that does not have to do anything with our actual ListPanel component.
Tricky part I guess is that where to put things for such custom component, right?
My intention going forward is to have src/pages/ (you can already see example and submissions folder there), where any page specific component would go.
My suggestion in this case would be just to create folder in src/pages/workflow and create something like PublicationSectionJats.vue.
On PHP side you can remove that ListPanel and couple of props (its hugely reduced when translations goes away) you can pass without using JatsListPanel.php.
Feel free to ask follow-up questions. I am also in process on figuring out details how to organise things, so there is always something I might have missed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jardakotesovec, yes, placing and being able to use the component was the problem for me, so I left it in the list panels. You are right, never the less, that should not be there. So I made the following changes:
- Moved the old
JatsListPanel
to the where you recommended, namelypages/workflow/PublicationSectionJats.vue
- Referenced this component into the
WorkflowPage.vue
, instead of theimport JatsListPanel from '@/components/ListPanel/jats/SubmissionJatsListPanel.vue';
- Moved the
PKP/classes/components/listPanels/JatsListPanel.php
toPKP/classes/components/PublicationSectionJats.php
- Removed the references to
ListPanel
fromPKP/classes/components/PublicationSectionJats.php
making the appropriate changes to make that work. - In
PKP/js/load.js
I referenced theimport PublicationSectionJats from '@/pages/workflow/PublicationSectionJats.vue';
- In
PKP/js/load.js
I registered the new component as suchVueRegistry.registerComponent('PkpPublicationJats', PublicationSectionJats);
- In
APP/templates/workflow/workflow.tpl
I changed the appropriate tab's tag from<jats-list-panel>
to<pkp-publication-jats>
With those steps we have the new component free of dependences from ListPanel
which can be placed (I think) independently where ever we need to, keeping the same functionality as before.
Please check if that meets our goals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@defstat That looks great, only thing that I added as commint to pkp-lib is to not include it to lib/pkp/js/load.js as its not general component, its fine just to imported in workflow.vue in 'components'.
@defstat Do you have some use case in mind? We need to handle the error situations coming from API, which should be covered by that And than if you would be doing something, lets say processing some xml on client side (lets not do that) that might fail on client side, than you should handle that with try catch. Don't see anything like that in this PR. Everything else would fall to category programmer mistake that crashes app. And there are probably opportunities for some custom handling of such crashes, but that would be up to me to implement global handler for unhandled exception. Thats not something that needs to be worried about in individual components. |
@@ -65,7 +65,7 @@ | |||
:files="newJatsFiles" | |||
:options="options" | |||
:queryParams="{fileStage}" | |||
:uploadProgressLabel="uploadProgressLabel" | |||
upload-progress-label="Uploading {$percent}% complete" | |||
@updated:files="setJatsFile" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also will need to be localizable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to :upload-progress-label="t('submission.upload.percentComplete')"
@@ -1,6 +1,7 @@ | |||
<script type="text/javascript"> | |||
import Page from './Page.vue'; | |||
import ContributorsListPanel from '@/components/ListPanel/contributors/ContributorsListPanel.vue'; | |||
import JatsSection from '@/pages/workflow/PublicationSectionJats.vue'; | |||
import Composer from '@/components/Composer/Composer.vue'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep naming consistent and import it as PublicationSectionJats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@defstat This is looking great, css still could be bit more refined, but lets do it as part of conversion to composition api and tailwindcss :-). I added only two small notes, nothing significant. |
No description provided.