-
Notifications
You must be signed in to change notification settings - Fork 433
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
Adding Configurable "Entities" to DSpace 7 (Part 1: Search/Browse, Display) #372
Conversation
…type/viewmode combination
W2p 52212 item display relations See merge request contributions/dspace-angular!1
…n' into 'entities-or2018' 52425: Publication - JournalIssue relation See merge request contributions/dspace-angular!2
I tested it again. Everything looks fine |
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've made a first review, I can't login using the entities REST demo server so I can't test entire application.
Generally I tested the UI and most of it works well, I didn't found any issue.
I've added a few inline comments in this review.
/** | ||
* The view-mode we're currently on | ||
*/ | ||
viewMode = VIEW_MODE_FULL; |
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.
It should be better to use an enum instead of a constant, to easily group all view mode available
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.
Changed
) | ||
) | ||
); | ||
|
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.
there are a lot of exported functions, maybe I would move them to an utils 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.
Done
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.
thanks for creating utils file. there is a reason why relationsToRepresentations
was not moved to item-relationships-utils.ts
?
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.
Likely @Atmire-Kristof didn't move it because it wasn't used anywhere else.
I've moved it now, as part of #390
import { Observable } from 'rxjs'; | ||
|
||
@Injectable() | ||
|
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.
Please add typedoc comments for this class
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.
Added
* @param defaults Default search options | ||
* @returns {Observable<SearchOptions>} | ||
*/ | ||
getSearchOptions(defaults: any = {}): Observable<SearchOptions> { |
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 method seems it is not used. if is not necessary I'd remove it with all related methods
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.
Removed
* @param defaults Default paginated search options | ||
* @returns {Observable<PaginatedSearchOptions>} | ||
*/ | ||
getPaginatedSearchOptions(defaults: any = {}): Observable<PaginatedSearchOptions> { |
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 method seems it is not used. if is not necessary I'd remove it with all related methods
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.
Removed
@@ -30,11 +31,23 @@ export class SearchResultsComponent { | |||
* The current configuration of the search | |||
*/ | |||
@Input() searchConfig: SearchOptions; | |||
@Input() sortConfig: SortOptions; |
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.
Please add typedoc comment
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.
Added
@@ -30,11 +31,23 @@ export class SearchResultsComponent { | |||
* The current configuration of the search | |||
*/ | |||
@Input() searchConfig: SearchOptions; | |||
@Input() sortConfig: SortOptions; | |||
@Input() viewMode: SetViewMode; |
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.
Please add typedoc comment
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.
Added
@@ -30,11 +31,23 @@ export class SearchResultsComponent { | |||
* The current configuration of the search | |||
*/ | |||
@Input() searchConfig: SearchOptions; | |||
@Input() sortConfig: SortOptions; | |||
@Input() viewMode: SetViewMode; | |||
@Input() fixedFilter: string; |
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.
Please add typedoc comment
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.
Added
@Input() sortConfig: SortOptions; | ||
@Input() viewMode: SetViewMode; | ||
@Input() fixedFilter: string; | ||
@Input() disableHeader = 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.
Please add typedoc comment
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.
Added
@@ -75,6 +76,7 @@ export class SearchConfigurationService implements OnDestroy { | |||
* @param {ActivatedRoute} route | |||
*/ | |||
constructor(private routeService: RouteService, | |||
private fixedFilterService: SearchFixedFilterService, |
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.
Parameter fixedFilterService is not described in typedoc, please add it
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.
Added
61947: atarix83 feedback changes 19-04-2019
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.
…ype from the item
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 reviewed again this PR.
it's good to me, I only added an inline comment, after it's been resolved this PR can be merged for me
[Configurable Entities] new data tweaks
Conflicts: config/environment.default.js resources/i18n/en.json src/app/+search-page/paginated-search-options.model.ts src/app/+search-page/search-filters/search-filter/search-filter.service.spec.ts src/app/+search-page/search-options.model.ts src/app/+search-page/search-page.component.spec.ts src/app/+search-page/search-page.component.ts src/app/+search-page/search-page.module.ts src/app/+search-page/search-service/search-configuration.service.ts src/app/core/cache/response.models.ts src/app/shared/object-list/search-result-list-element/item-search-result/item-search-result-list-element.component.spec.ts src/app/shared/object-list/search-result-list-element/search-result-list-element.component.ts src/app/shared/object-list/wrapper-list-element/wrapper-list-element.component.ts src/app/shared/services/route.service.ts
[Configurable Entities] final master merge
As this is already at +2, and merge conflicts (with MyDSpace PR) have now been resolved, I'm merging this immediately. |
Overview
This PR is the first phase of work from the DSpace 7 Entities Working Group.
It creates the Angular UI features corresponding to the Entities REST API/Backend PR: DSpace/DSpace#2376
Additional details on the design, use cases and screenshots of these user interface screens are available in a Google Document.
This PR provides the following features to the Angular UI:
This PR does not provide the following (these will be provided in a future PR):
Reviewing
As you can see in the commits, nearly all code written in this PR was written by Atmire. However, the DSpace 7 Entities Working Group used the same code review processes established by the DSpace 7 Working Group.
Obviously, other code reviews are always welcome. But, because of the large size of this PR, you are also welcome to provide a detailed review of the following (which together provide a deeper understanding of the feature this PR adds):
Additionally, if you would like to see/review all the past reviews, each of the smaller PRs that make up this larger PR are available in GitHub via this search: https://github.com/DSpace/dspace-angular/pulls?utf8=%E2%9C%93&q=is%3Apr+base%3Aconfigurable_entities+is%3Amerged
Testing
This PR can be built and tested locally. However, as you will be unable to create new Entities, there are a few options available for testing.
Option #1: By default, if you download & build this PR following the PR testing procedure in our README, then you will find your
environment.default.js
is updated to point at our Entities REST API Demo site (https://dspace7-entities.atmire.com/rest/#/rest/api). This demo site provides the backend (and test data) provided in the corresponding REST API PR DSpace/DSpace#2376Option #2: If you would rather install the backend (and test data) yourself locally, follow the recommended Testing notes in the corresponding REST API PR DSpace/DSpace#2376 (In this PR, we've provided a test data set to use for local testing)
Acknowledgments
This PR was the collaboration of many individuals (over the span of several PRs to the
configurable_entities
branch), including @artlowel (developer), @Atmire-Kristof (developer), @LotteHofstede (developer), @benbosman (developer), @antoine-atmire (developer), @paulo-graca (reviewer), and @tdonohue (reviewer). Many others were also involved in the conception/design/discussion in various DSpace 7 Entities Working Group meetings.A full list of the smaller PRs that make up this PR is available at: https://github.com/DSpace/dspace-angular/pulls?utf8=%E2%9C%93&q=is%3Apr+base%3Aconfigurable_entities+is%3Amerged