-
Notifications
You must be signed in to change notification settings - Fork 11
CORE-103: Allow DE metadata dialogs to attach AVUs to other AVUs #394
Conversation
Also updated layout of metadata list action buttons and edit dialog child AVU expansion panel.
Also add close confirmation dialog for unsaved changes. Also some minor DEConfirmationDialog refactors, including adding the `aria-labelledby` prop, and to allow intl components as the message prop. Removed obsolete ConfirmCloseDialog in favor of DEConfirmationDialog.
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'm leaving some PR comments in a bulk "review" so I don't spam your slack or email 😆
Note that this PR also has changes to some Metadata Template components, but those a fairly minor changes to remove some material-ui deprecation warnings and to replace the obsolete ConfirmCloseDialog
with instances of DEConfirmationDialog
(which itself got some updates).
view.unmask(); | ||
view.closeMetadataDialog(); | ||
|
||
dialog.setModal(true); |
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.
The presenter will set the SelectMetadataTemplateDialog
as modal now, since the React metadata dialog is hidden while showing this GXT dialog, in order to avoid custom CSS on this SelectMetadataTemplateDialog
to keep it on top.
Maybe keeping the React dialog open under this SelectMetadataTemplateDialog
by adding custom CSS would be preferred instead?
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 am ok with hiding the React dialog and setting this Modal
. I see no real benefit of keeping it open especially if you need add CSS 😭.
closeMetadataTemplateDialog(); | ||
} | ||
|
||
@Override | ||
public void closeMetadataTemplateDialog() { | ||
templateViewDialog.closeDialog(); | ||
view.showMetadataDialog(); |
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.
Since the metadata dialog is hidden when opening the template selection dialog, we reopen it here after closing the template view.
onTemplateSelected(selectedTemplate.getId()); | ||
} | ||
} else { | ||
view.showMetadataDialog(); |
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 will re-show the metadata dialog if the SelectMetadataTemplateDialog
is closed without opening a template.
|
||
componentDidUpdate(prevProps) { | ||
if (prevProps.targetResource !== this.props.targetResource) { | ||
// The presenter wants to load metadata for a different target, |
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 happens when the dialog is first opened with a loading
mask and the targetResource
(in order to display its name in the dialog header), then the metadata is loaded from the service later. The GWT view will also reset the targetResource
to prevent the form from assuming the newly loaded metadata is "dirty".
See also DiskResourceMetadataViewImpl#loadMetadata
.
); | ||
}; | ||
|
||
export default withFormik( |
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 had to remove enableReinitialize: true
here, otherwise when metadata is updated from a template, Formik would reset the form, then the metadata form values would no longer be "dirty", and the user would not be able to save their changes.
So resetting the form is done manually in componentDidUpdate
, and only when the targetResource
changes.
|
||
<FieldArray name="avus" | ||
render={arrayHelpers => ( | ||
<FormDialogEditAVU {...arrayHelpers} |
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.
The MetadataList
above is the listing grid for each AVU.
One of these FormDialogEditAVU
components will open when clicking an Edit
icon from the grid.
<FieldArray name="irods-avus" | ||
render={arrayHelpers => ( | ||
<FormDialogEditAVU {...arrayHelpers} | ||
editable={false} |
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.
iRODS AVUs are not editable.
<span> | ||
<IconButton id={build(ids.EDIT_METADATA_FORM, ids.BUTTONS.SAVE_METADATA_TO_FILE)} | ||
aria-label={formatMessage(intl, "saveToFile")} | ||
disabled={loading || (dirty && editable) || isSubmitting} |
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.
As mentioned in the PR description, this Save to File
icon will be disabled if the metadata is "dirty", so the user is not mislead into thinking any unsaved changes will be exported to their saved file.
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 didn't find any major issues. However there is one sticking point I would like to bring up is the placement of close
button on the left side rather than the usual right side for the DE. While I understand why this was done, there are several ways to provide save
, download
template etc. from a dialog. For example, we can add a https://material-ui.com/demos/bottom-navigation/
and have the dialog body scroll or we could just add just a dot menu to dialog header with all the options or we could add a Speed Dial https://material-ui.com/lab/speed-dial/ which could display all these related actions (although this is still in labs). This way we can keep the close
button on the right side.
.../iplantc/de/diskResource/client/presenters/callbacks/DiskResourceMetadataUpdateCallback.java
Outdated
Show resolved
Hide resolved
view.unmask(); | ||
view.closeMetadataDialog(); | ||
|
||
dialog.setModal(true); |
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 am ok with hiding the React dialog and setting this Modal
. I see no real benefit of keeping it open especially if you need add CSS 😭.
umd.setOtherMetadata(view.getAvus()); | ||
drService.setDiskResourceMetaData(resource, umd, batchAvuCallback); | ||
view.closeMetadataDialog(); | ||
})); |
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.
✨
import SaveIcon from "@material-ui/icons/Save"; | ||
import SaveAltIcon from "@material-ui/icons/SaveAlt"; | ||
|
||
const ConfirmationDialog = withI18N(DEConfirmationDialog, intlData); |
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.
👍
padding: "none", | ||
}, | ||
]; | ||
|
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.
✨
Oh right, I was originally modelling these dialogs off the full-screen dialog example, but these dialogs are no longer full-screen. I like the idea of having the cancel/save buttons at the bottom-right, as we do in all other normal-size-dialogs. So I can add those bottom-right buttons and move the close button to the top-right in this dialog, and in the metadata template dialogs as well. |
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 only had a few minor comments - LGTM
.../main/java/org/iplantc/de/diskResource/client/presenters/metadata/MetadataPresenterImpl.java
Outdated
Show resolved
Hide resolved
heading: PropTypes.oneOfType([ | ||
PropTypes.string, | ||
PropTypes.object, | ||
]).isRequired, |
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.
👍
Moved Save buttons from top-right to bottom-right of dialog, added Close buttons next to the Save buttons, and moved Close icon buttons from top-left to top-right.
Refactored to use TableSort util functions, and to only sort TableRow components in the MetadataList render method, rather than sorting AVU lists in the metadata model. This allows sorting without making the metadata form "dirty".
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.
Sorting LGTM!
Sorting changes LGTM. |
Refactor MetadataList unsorted TableRows into AVURow components for readability and a smaller render method.
Thanks for the reviews! 👍 |
This PR will replace the GXT view for the Edit Metadata dialog with a React view that supports nested metadata:
Note that hovering over the
Delete
icon in the listing grid will highlight the icon a red color:After the dialog title are the
View in Template
,Save to File
,?
(Help), andX
(Close) icons:Selecting the
?
(Help) icon will display a help-text popup:The
Save
button in the bottom-right of the dialog will only become active if the metadata has been updated in some way, which will also disable theSave to File
icon. TheSave to File
icon will be disabled since the service endpoint that exports the metadata to a file will only use metadata that is saved in the database. So disabling thisSave to File
icon is new in this PR, since I feel it's misleading to the user if they make some changes to the metadata, then decide to export it without saving the changes first.Selecting the
X
icon orClose
button when there are metadata changes will display a confirmation dialog:The form for editing an AVU and its children:
The form for editing a child (nested) AVU:
If there are also iRODS metadata, then the tabs
User Metadata
andAdditional Metadata
will appear under the dialog's header (otherwise the tabs are hidden if there are no iRODS metadata). TheView in Template
icon is replaced with anImport
icon when clicking on theAdditional Metadata
tab (unless the data is read-only). TheImport
icon is only active if at least 1 row of iRODS metadata is selected:Note that iRODS metadata is read-only, so the
Edit
andDelete
icons in the listing grid are replaced with a singleView
button.Selecting the
Import
icon will display a confirmation dialog before attempting to import the metadata:For read-only files/folders (such as public data), the Metadata dialog has a read-only
View
mode (there are no iRODS metadata in the following example):Note that the
Save
button is hidden, and just as in the iRODS listing grid, theEdit
andDelete
icons in the listing grid are replaced with a singleView
button.The form for viewing a read-only AVU and its children:
The form for viewing a child (nested) AVU (without any child AVUs under it):
Finally, the view for a read-only file/folder with iRODS metadata: