Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Content reports ported from DSpace 6.x #2163
Content reports ported from DSpace 6.x #2163
Changes from 11 commits
f37122b
1d1c713
be97cad
bc73039
9533713
9677de7
97c6bf5
18f913e
2b8c377
a105dd5
2c0e0ab
5a01670
75ca560
1cdbfc5
0e2b767
de83b35
6d9768e
ef651a3
16708d6
d686cb1
5f88a07
4cd20d6
85630ba
872a670
cadd1df
998dd94
2d2a74a
8bcd82e
27a9ce5
2a65bd8
79993ce
513b28b
64e3149
f1cfe99
5f41bc2
1c6216d
c764950
0497991
e2bf9d2
1cea469
600c991
91d97f8
ba5f50e
92553e0
26caab1
410a278
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Rather than defining odd and even coloring yourself, please use bootstrap classes for striped rows. That way they can be themed
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.
Should be OK.
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. This simplifies my code indeed.
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.
Same here, I'd avoid the custom table styling, and use bootstrap's table style instead. It'll make things more consistent
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.
Will need some guidance on this one.
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.
If you use the classes
table
andtable-striped
on the table the header row will automatically get a pronounced bottom border, as you can see in the examples in the docsThere 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.
In order not to duplicate code like this all across the app, and to be able to cache responses, please use a Dataservice for this instead. While a request like this might have been a POST(i.e. a form submit) in dspace 6, with a REST api, POSTs don't make a lot of sense in this context, as you're not creating anything. This should be a GET endpoint. And when it's a GET you can likely get away with simply extending
HALDataService<FilteredCollections>
in your DataService.The FilteredCollection model will also need cerialize annotations (basically put
@autoSerialize
above each property that comes from rest), and then you don't need to (de)serialize it manually anymore either.Same for FilteredCollections, however because collections and summary are complex types you'll likely need an
@autoserializeAs(FilteredCollection[])
for thoseYou'll also need to add a property for the type (I see that's already in the rest response, just not in the model in angular) and add the @typedObject annotation to the class, a static type property as well as a regular type property, as you can see here
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.
We will need guidance here too. We tried at first to build some DataService, but to no avail, so we decided to use lower-level REST queries directly in the component. If we can move this code to a full-featured DataService, of course it might be better.