Cst 3782 fix repeatable fields #1025
Conversation
…the end of the repeatable fields array
|
This pull request introduces 1 alert when merging 42636c6 into f3246fb - view on LGTM.com new alerts:
|
…le-fields # Conflicts: # src/app/shared/form/builder/ds-dynamic-form-ui/relation-lookup-modal/dynamic-lookup-relation-modal.component.spec.ts # src/app/shared/form/builder/ds-dynamic-form-ui/relation-lookup-modal/dynamic-lookup-relation-modal.component.ts
|
This pull request introduces 1 alert when merging 8e7fad4 into 554dcde - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
@alemarte : I gave this a quick test today, and it seems to work (I'll admit, I only tried the basics so far though & need to do a more thorough test next week). I did notice a few small usability issues that I think we should clean up;
- The "hint" text always appears under the first repeatable field, which looks odd to me. I think it should appear under the last repeatable field, in the same way that the "+ Add more" button always appears at the end of the list.
- When dragging the last item, when you grab the item, it "appears" as though you are also dragging the "+ Add more" button. This is a minor issue, as the "+ Add more" button doesn't move after reordering...it just looks odd
I also noticed one small misspelling in the new code. Overall though, this looks good at a glance. I admit, I haven't yet tried the "Lookup" feature again though -- will do so next week.
UPDATE: I tested the "Lookup" feature today (Monday, Feb 22). It mostly works, but I've found a few issues:
- As detailed in the original ticket, we had talked about having the "Lookup" feature appear next to each value in a multi-valued field: #971 (comment) Currently, it's just appearing a the bottom of the list, which limits you to only doing a "Lookup" of the last value in the list.
- Clicking "Lookup" does bring the value into the lookup popup so that I can search on it. That works. However, once I select a value in the popup, I end up with an error:
Unable to add relationship: No suitable match could be found for relationship type isAuthorOfPublication between the two itemsBut, I think this might be an existing error (i.e. may not be a fault of this PR), as I cannot seem to get this "Lookup" popup working on the demo site either. So, we could log this as a separate bug, unless you figure out an easy way to fix it.
UPDATE 2: Some more issues I just found:
- This appears to have broken the "Subject Keywords" field. I no longer can add keywords by pressing "Enter"
| arrayCounter++; | ||
| } else { | ||
| const fieldArrayOfValueLength = this.getInitValueCount(arrayCounter - 1); | ||
| const fieldArrayOfValueLenght = this.getInitValueCount(arrayCounter - 1); |
There was a problem hiding this comment.
Tiny thing...you accidentally changed this to be misspelled. The correct spelling is the original version fieldArrayOfValueLength
|
This pull request introduces 1 alert when merging 975ef7e into 2ff232f - view on LGTM.com new alerts:
|
|
@alemarte : Apologies, but it looks like this PR has a minor code conflict now. Could you rebase this on |
…le-fields # Conflicts: # src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.scss
|
This pull request introduces 1 alert when merging b3afaa5 into 11b6b3c - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
👍 Thanks @alemarte . I retested this today, and the issues I previous reported are now resolved. So, I feel this is good enough to merge.
That said, I did want to note that I've noticed two new bugs in the Submission UI, but I don't think either of these are directly related to this PR. So, they could be logged & fixed separately as needed (unless you know of a quick fix & want to implement it).
- The first issue is that creating & linking Entities via the "Lookup" option (e.g. next to the Author field) no longer seems to work. This is related to what you described in your comment. Simply put, if you click Lookup, select an Author via ORCID, and select to create a new Entity, the process claims to succeed...but that Author record doesn't appear under the Author field in the Submission UI. However, this same issue occurs on the Demo Site (https://demo7.dspace.org/), so it doesn't seem to be caused by this PR & probably should be logged as a separate ticket
(I can add a new ticket assuming others agree)(In a meeting we determined this was a configuration issue on the demo site, as the Lookup field should not be configured for all Collections. It only can be used in Collections supporting Entities) - The other issue is with the "Subject Keywords" field. I can verify that #1020 is now fixed...however, it appears we no longer can add Keyword which are not in a Controlled Vocabulary. If I select an entry from the Controlled Vocabulary, then I see that Keyword added. But, there seems to be no way to add other keywords -- I expected to be able to type a word and click Enter (or similar) and see them also be added. But again I'm not sure if this is caused by this PR, and it could be fixed in a follow-up as needed
(I can add a new ticket assuming others agree)(In a meeting we determined this was cause of a configuration setting on the demo site, as the Keywords field is configured to limit to only entering controlled vocabulary terms.)
There was a problem hiding this comment.
Thanks @alemarte!
The code looks good, and the feature works well 👍
I did notice a few issues though, most having to do with fields that mix entities and metadata values.
All tests were performed in prod mode, in this collection on the api7.dspace.org backend. All tests use the author field, unless mentioned otherwise.
Alert while dragging
When you pick up a row in this PR to drag it, it changes into an alert that says "Drop the item in a new postion". That didn't use to be the case. You used to simply see the row you were holding as it was.
Having the thing you picked up change as you drag it is disorientating in my opinion.
Search query stored as metadata value
If you enter a search query and click the lookup button, and then select an entity and create a relationship, the field with the search query disappears and the save button becomes active.
If you save, the field with the query reappears and seems to have been stored as metadata on the item
Saving empty field issue
If you enter a value in a repeatable field, then click "add more", the save button is disabled: it hasn't detected any changes. So far, so good.
However if you drag that empty field to the top, without filling it in, the save button becomes active, and if you click save you get an error. (even if you've also made other changes that aren't just empty fields)
Subject Keywords unclickable
The subject keywords field can't be clicked (to start typing in it) most of the time. You can however select a different field and tab to it, to select it
Reordering in the identifiers field is confusing
Because the identifiers fields are all in a single reorderable list I got the impression reordering wasn't working at first.
On closer inspection it seems that the values for each kind of identifier (ISSN, Uri, Other, etc) do retain their order, but you're also free to mix different kinds of identifiers. You lose that mixed order if you reload the edit page.
The order between values of e.g. dc.identifier.issn and dc.identifier.other are of course meaningless, there's no need to store them, but the UI makes it seem like you can.
It would be better if each type of identifier had its own reorderable section, and they couldn't be mixed.
Missing hint
If an entity is the bottom value of a repeatable field, the hint isn't shown. It will reappear if you drag a metadata value to the bottom.
Indent
Repeatable fields are ever so slightly more indented than other fields, which makes the form look untidy
|
Thanks @artlowel for your points, they were all good. I tried to address/fix all of them.
We opted to generalize the drag and drop placeholder because with the new repeatable fields implementation the old display strategy appeared to be even weirder, since together with the field value the lookup button and the add-more button were dragged too.
This was a regression and has been fixed (basically by restoring the submission save during the opening of the lookup modal).
This turned out to be a very big problem. This problem is also present on the demo, and is mainly due to the fact the the move operation only sees the current form status, without considering possible empty values in the middle. A way to simply prove that is by loading a submission starting with 3 saved author. Then:
The move operation wrongly sees the array length as 3, but the previous patch reduced to 2.
Is related to #1020 and will be fixed by #1057
This is weird I agree, but is the current demo behaviour and in the context of this PR we have two choices in my opinion. Enable the dragging as is or disable it completely for the qualdrop scenario. I've added a configuration field to the DynamicRowArrayModel that allows us to conveniently disable the drag and drop feature and exploited it for cases such as the identifier form control. See here
The missing hint in case of an entity at the bottom has been added.
This has been restored (a merge conflict was not handled properly) |
artlowel
left a comment
artlowel
left a comment
There was a problem hiding this comment.
Thanks @alemarte!
Alert while dragging
We opted to generalize the drag and drop placeholder because with the new repeatable fields implementation the old display strategy appeared to be even weirder, since together with the field value the lookup button and the add-more button were dragged too.
To experiment the effect just comment out the *cdkDragPreview. Since we are operating at the outer array level, we don't have an easy way to create a preview which contains the dragged value in a sufficient generalized manner, suitable for all the possible models. We can of course manage that, but we are running out of time and restoring this could easily be a following up improvement.
I think it works more intuitively without the cdkDragPreview template. You can hide the buttons and the hints while dragging by specifying a cdkDragPreviewClass and setting buttons and hints to display: none. I've done so in this commit: atmire@a1884b5
One less than ideal thing about that is that you have to do it in the global css as this spans multiple components, but I don't think that's a big problem if the css class is specific enough.
Saving empty field issue
This turned out to be a very big problem. This problem is also present on the demo, and is mainly due to the fact the the move operation only sees the current form status, without considering possible empty values in the middle. A way to simply prove that is by loading a submission starting with 3 saved author. Then:
- empty the author in the middle
- drag the last author to any other position
- save
The move operation wrongly sees the array length as 3, but the previous patch reduced to 2.
Fixing this by tuning the patch move operation generator is sincerely a world of pain. We don't have a way to monitor and predict the user behavior and there are too many cases to handle.
So we change approach. We tried to apply a much simpler approach by patching every time the value of the entire array. It works, smoothly 100% of the time.
As a side note about patching the entire array, we found out that it doesn't work when at least virtual value is present. So this will need a fix rest side. (already estimated to 8h-12h, and we could claim the task after beta5 during the testhaton).
I agree patching the entire array is the better solution. However breaking this virtual values in repeatable fields right before testaton might not be the best plan either. So perhaps we should rollback this fix for now, work on the backend during testathon, and release this fix as a separate PR when the rest work is done too. @tdonohue, what do you think?
We discussed this in today's meeting @abollini will ensure that the Rest PR is ready before testathon.
Reordering in the identifiers field is confusing
This is weird I agree, but is the current demo behaviour and in the context of this PR we have two choices in my opinion. Enable the dragging as is or disable it completely for the qualdrop scenario. I've added a configuration field to the DynamicRowArrayModel that allows us to conveniently disable the drag and drop feature and exploited it for cases such as the identifier form control. See here
I wouldn't disable it, I'd just create a ticket for it and come up with a better approach for this either during testathon or when we work on the submission for 7.1
done |
…le-fields # Conflicts: # src/styles/_global-styles.scss
4 participants
References
Fixes #971
The change in behavior also fixes several usability tickets (verified by @tdonohue):
Fixes #864
Fixes #884
Fixes #948
Fixes #949
Fixes #931
Fixes #1000
Fixes #950
Description
This PR aims to remove the template row from the repeatable field control, in order to tackle numerous side effects that have arisen with such an implementation.
Most of the changes consists in reverting to the previous beheviour of the form array control.
Then the lookup functionality have been extrapolated from the form container (which should have very bounded responsibilities) to the customization of repeatable fields in the form component.
Instructions for Reviewers
Summarizing the results of this PR, we can make the following considerations:
We have the possibility to increment the numbers of the repeatable field values, by clicking on the Add More button.
We can delete values by clicking on the Trash button.
We can drag and drop values in order to sort them.
Since we don't have anymore the fake template row on the top of the control, this solution doesn't allow the user to direct inject the query text into the lookup modal.
In our opinion clicking on the Lookup button and then write the text criteria inside the modal brings us to a very similar experience and so it could be a very good compromise.
Checklist
yarn run lintpackage.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.