Conversation
|
Hi @kshepherd, |
5b7602f to
6c6f045
Compare
|
I'm trying to limit the number of new properties on the onebox component, which is already quite a monstrous little thing. I combined the suggestion api and the vocabulary service into a single observable, and I want to combine the suggestrt/suggestrte templates with the existing rt and rte templates. Other than that, this is working as expected. I may adjust the spinner stylistically. |
bca4d58 to
48550ca
Compare
|
This is working the way I want it to. I made large changes to the onebox component, and I plan on adding more tests for it, as well as tests for the new search service method for getting suggestions. The existing tests should all pass (famous last words) |
|
@strawburster : Just to clarify, is this now ready for review? If so, we should move this PR out of "Draft" status . If you cannot do so, I can do that on your behalf. |
|
@tdonohue I'm not sure. I feel that this is done on the frontend, but the PR depends on DSpace/DSpace#10855 which is not ready yet. Also, I want to add tests but I suppose that can be part of the review. If you feel that it's appropriate you can change it for me, I do not have access because it is Kim's work. |
|
@strawburster : If this depends on another PR which is not yet ready, then it sounds like a test & review may not be easy to achieve. Once the other PR is ready, I can remove the "draft" status from both PRs at the same time. Just let me know when to remove the draft status. |
|
I've added the checklist template back into the issue description here to help us show review / merge readiness |
d8f1178 to
4a9cbfe
Compare
|
I'm sorry I can't edit the checklist, but linting, circular dependencies, typedocs, i18n, and the last two, could be checked by now. The service implementation for suggestions is tested as far as I think I need to because of its simplicity, but I am still adding more tests to the onebox component for when the vocabulary is of type 'suggest'. I expect one of the unit tests to fail, because there needs to be a hypermedia link between the discover and suggest endpoints before I can use the HAL service to look it up. This is a reminder to myself to do that once it becomes available in the backend. |
|
I've added onebox component tests for the case where the vocabulary type is 'suggest'. Please check the box for tests as well now, pending further review. |
935b754 to
9e5a9dd
Compare
| } | ||
| } | ||
|
|
||
| if (this.model.vocabularyOptions.type === 'suggest') { |
There was a problem hiding this comment.
Maybe you can use here if (this.isSolrSuggest) {?
| this.dispatchUpdate(event.item); | ||
| } | ||
|
|
||
| onSelectSuggestedTerm(event: NgbTypeaheadSelectItemEvent) { |
There was a problem hiding this comment.
Is this used? The onSelectItem is used in the https://github.com/DSpace/dspace-angular/pull/4439/changes#diff-377e31db1a603b93958cf2e2d7ba4b7366281905e4b17da965ca9913294b0dbfR87
src/app/shared/form/form.reducer.ts
Outdated
| fieldIndex: action.payload.fieldIndex, | ||
| message: action.payload.errorMessage, | ||
| }; | ||
| console.dir(error); |
There was a problem hiding this comment.
Is it necessary to keep this? It will log every form validation error object to the browser console.
| AsyncPipe, | ||
| AuthorityConfidenceStateDirective, | ||
| FormsModule, | ||
| FormsModule, |
There was a problem hiding this comment.
Duplicity import? FormsModule and NgbTypeaheadModule
| provideMockStore({ initialState }), | ||
| provideMockActions(() => new Observable<any>()), | ||
| { provide: VocabularyService, useValue: vocabularyServiceStub }, | ||
| { provide: SearchService, useValue: SearchServiceStub }, |
There was a problem hiding this comment.
useValue: searchServiceStub not SearchServiceStub
9e5a9dd to
0015a82
Compare
|
Thanks for the review @milanmajchrak ! They are all good suggestions. I'm curious if you have had a chance to test this yourself? |
0015a82 to
9dacac0
Compare
I was only involved in the testing to some extent, while my colleague did most of it. She was able to run it successfully, and then we went through it together. She also verified that the functionality works in a comparable way to our CLARIN-DSpace customization, and we were satisfied with the results. |
|
Hi @kshepherd, |
| "form.other-information.orcid": "ORCID", | ||
|
|
||
| // "form.other-information.weight": "Weight", | ||
| "form.other-information.weight": "Hmotnost", |
There was a problem hiding this comment.
We discussed this translation with UFAL and concluded that it is not correct in Czech. The correct wording is Váha. Could you please fix it in this PR?
Adds Solr-based suggestion to dynamic onebox vocabulary lookups.
See DSpace/DSpace#10855 for more notes
Rest Contract: DSpace/RestContract#328
UI
In the dynamic-onebox Angular component which traditionally handles vocabulary / authority display like tree lookup or autocomplete, if the vocabulary type is "suggest", it will use a different search method and display template for the suggest terms. If not, the current behaviour is used (lookup / suggest from XML / etc)
Like the backend, the actual suggest term JSON is not fully modelled in Angular, it is just treated as JSON. This is done to keep things light (they are not proper DSpace addressable objects or anything, not used anywhere but as input providers) and due to the structure of the JSON where terms are used as key names, etc.
Developed by The Library Code with the support of Universität Bremen and GESIS.
Checklist
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.