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

pkp/pkp-lib#6062 CrossRef and DOI UI/UX quality of life improvements #134

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ewhanson
Copy link
Contributor

Draft pull request for preliminary code review.

Base automatically changed from master to main February 18, 2021 01:53
class="pkpFormField__description"
v-html="description"
:id="describedByDescriptionId"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this isn't a general field that will be used elsewhere, the <tooltip>, <span>, <help-button> and description elements are not needed.

:id="multilingualProgressId"
:count="multilingualFieldsCompleted"
:total="locales.length"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

The prefix and <multilingual-progress> elements are also not needed.

classes.push('pkpFormField__control--hasPrefix');
}
return classes;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

controlClasses and classes are not needed either, I think.

*/
setFocus() {
this.$refs.input.focus();
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this isn't needed since it's only used with the prefix option (whcih can be removed)

/**
* Submit the DOI edits
*/
triggerDoiSave() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just call this method save().

* @param {Number} itemId
* @param {Boolean} isSelected Item selection status
*/
toggleDoiSelected(itemId, isSelected) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A way I like to do this to make the code a little more expressive is to pass in the new status. So when I see toggleX(), I think that it will turn it on/off depending on its current state. But in this case you are explicitly passing in the state that you want.

I'd change the method to work like this:

/**
  * Select a DoiListItem in the DoiListPanel
  *
  * @param {Number} itemId
  * @param {Boolean} select Whether it should be selected or deselected
  */
selectItem(itemId, select) {
	const selected = this.selected.includes(itemId);
	if (select && !selected) {
		this.selected.push(itemId);
	} elseif (!select && selected) {
		this.selected = this.selected.filter(item => item !== itemId);
	}
},

And then pass the desired state in when I call the method (notice the !):

this.$emit('selectItem', this.item.id, !this.isSelected);

toggleDoiSelected(itemId, isSelected) {
if (isSelected) {
if (!this.selected.includes(itemId)) {
// Add to selected
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this comment or the removal one below.

*/
addFilter(param, value) {
let newFilters = {...this.activeFilters};
if (['status', 'isPublished'].includes(param)) {
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 isPublished is not used, is it?

},
filters() {
return [...this.computedFilters, this.issueFilter];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll want to move these filters out of the ListPanel and into the PHP code that passes the initial data into the ListPanel.


.doiListPanel__options--button {
margin-left: 0.25rem;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Because these first two selects have a prefix .doiListPanel__, they do not need to be nested inside of .doiListPanel { ... }. The others should stay nested inside .doiListPanel so they are not applied to other components.

WIP: pkp-lib#6062 DOI refactor
Squashed commits:
Start integration with DOI API endpoints
[e88b79a] Add template for depositor action section in DoiListItem
[83b9d52] Add notes on DOI API overhaul
[1cd01de] Update DOI saving workflow
[b3d3df3] Update ListPanel item actions styling
[c7baaa7] Clear selected items when items are reloaded.
[d70f103] Enable downloading exported XML through XHR API calls.
[0de3706] Reactor DOI editing across components
[5c23e19] Remove mutable items from DoiListPanel
[8c43150] Move DOI field logic to DoiLIstPanel and handle ListPanel items as mutable data objects
[a70852b] Update PreviewDoiListPanel to include previous DoiListPanel refactor changes
[048a739] Refactor item selection and expansion
[bc6ba91] Update filters and other styling
[a067c18] Update communication between DoiListPanel and DoiListItem around selection state
[1b08667] Update Crossref spelling
[8fa2a8a] Implement DOI management page suggestions and improvements
[eb904d9] Fix autosuggest filter not clearing empty filters
[5511083] Working demo
[c6fbb4a] Start work on depositing action
[ccc18fd] Start cleanup based on code review
[ebabdfe] Remove exclamation icon from deposit status badge
[a107b51] Update package-lock.json
[21e0d21] Limit DOI editing to enabled types (publication, issue, representation)
[0fee298] Remove old code and fix preview typos
[77caf24] Implement pagination and move info fetching logic into DoiListPanel.inc.php
[78e2e6a] Remove console logs from FieldDoiText.vue
[e481347] Add placeholder deposit dialog box
[ee2ff17] Update bulk actions dropdown
[27f6786] Use table instead of list for DOI editing
[c29bdf5] Update issue filter to use FieldSelectIssues
1451ca9[5a2cf43] Group bulk actions and make actions a single line
[f3e7216] Add localized fields
[bd028f6] Clean up documentation and code
[3c92eb1] DOI auto assignment, prepend DOI prefix in DOI edit field
[02df0b1] Fix FieldDoiTest button focus bug
[cba5ae7] Update filtering to work for submissions and issues
[92cad86] Handle empty issues on Issue DOI management page
[b15fd94] Complete first pass at issue DOI management
[0d13788] Add Crossref error display, update deposit status handling
[2f30a41451ca99] Enable only show crossref in DOI management if plugin enabled
[a3bc366] Add first pass at issue filtering
[a81af0b] Enable DOI editing from mangaement page
[939965b] Integrate DoiListPanel into OJS
[f02a257] Refactor list item format for use in OJS
[b9eeda3] Add filtering and update visual style
[5e69f1c] Create custom FieldDoiText component
[fd972bd] Update DOI management panel UI
[1926db5] Add improvements expander, issue handling
[8d768c9] Use submission/publication data object
[eeea20d] Add DoiListPanel with select all and expander
[3f04b8f] Copy announcements panel and add selection

Make EntityDAO-based DOIs editable from UI
WIP: Integrate EntityDAO-based DOIs into UI and remove Crossref specific references
WIP: Implement markRegistered functionality
WIP: Implement deposit error and registration message in abstract way
Make DOI interaction generic at pkp-lib level for reuse
WIP: Move DOI functionality out of plugins
WIP: Move more DOI functionality out of plugins
Update DOI-related constants for Vue components
Add in route-based export and mark registered for submissions and issues
Add issue and submission depositing flow + Crossref implementation
Remove old deposit/export code
Update API flow for export actions between backend and UI
Refactor bulk actions and add DOI assignment
Make DOI item title styling consistent across object types
Fix typo that prevented Issue DOI type from showing up in management UI
Simplify IDoiRegistrationAgency interface
Rename DOI object stored on pubObjects to 'doiObject' to avoid ambiguity between 'doi' (object) and 'doi' (string)
WIP: UI library code review changes
Setup Crossref plugin configuration checks
WIP: Add deposit all via queued jobs
WIP: Refactor to enable Datacite functionality
Work through TODOs
Add TemporaryFile-based DOI XML export downloads
Disallow deposits/exports for unpublished items
Update DOI components for testings
Wrap up first round code review changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants