-
Notifications
You must be signed in to change notification settings - Fork 46
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
pkp/pkp-lib#3594 Refactor library to improve docs, JS linting and dep… #27
Conversation
…endencies This is a complete rebuild of the UI Library as a Vue CLI 3 app. It caters to child components better now, which is what the majority of our components will be.
/** | ||
* Dummy constants required by components | ||
*/ | ||
const: { |
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 suspect these constants are at risk of falling out of sync with the OJS equivalents. I'd like them to be defined only in one place (either PHP-land or JS-land), and I'm not opposed to e.g. having a .json
file (or set of files or something similar) that is the source for both.
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.
(Also, it looks like you have ASSOC_TYPE_PRESS
, which is OMP-specific lingo, and not ASSOC_TYPE_JOURNAL
.)
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.
(Open to ideas on this -- it's dummy stuff, but it'll be a pain to debug when we change the values and have to track down an obscure bug here.)
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.
Totally agree. This has been one of those headaches of bringing things in piecemeal that I haven't got a good solution for right now.
Eventually, we will have a parent component that controls each page. So there will be a SettingsPage
, WorkflowPage
, IssuesPage
, SubmissionsPage
, and so on. These will be a kind of root component for each page, which will orchestrate data and components on the page.
When these are in place, I plan to remove this pkp
global and instead inject these dependencies (including the constants) into each page. In the same way that PKPTemplateManager
injects some global variables into our smarty templates, we'll do the same for the pages, which will then pass them down to the components as needed.
At that point, I think it will make sense to retain a registry of these required constants, since they can be pegged to a page that is actively using them. For now, we don't have a good way of tracking when a dependency is no longer used.
If you want, I can try to address this sooner rather than later. It may be best to simply inject these dependencies into each component, anyway. It would just involve some (minor) refactoring.
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.
(And ASSOC_TYPE_PRESS
is used by an OMP-only component, CatalogSubmissionsListPanel
, but nothing uses ASSOC_TYPE_JOURNAL
, so it's not needed there.)
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.
No need to code it right now, but it should definitely be filed against a particular release so it doesn't just disappear onto the scrap heap. I'd suggest filing against OJS 3.2 😄 and assigning to yourself
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.
pkp/pkp-lib#3594 Refactor library to improve docs, JS linting and dep…
…endencies
This is a complete rebuild of the UI Library as a Vue CLI 3 app. It
caters to child components better now, which is what the majority of
our components will be.