Refactor badges

[LotteHofstede]

Description

This PR bundles all different types of list/grid view badges into a single ds-component.
This way, badges will now be easier to move and align with other content.
This PR also added themed versions of certain components that hadn't been made themeable before.

The following components were made themeable:

• SearchFiltersComponent
• SearchSidebarComponent
• All the existing badge components

Instructions for Reviewers

This PR can be tested by comparing different types of list/grid view and making sure they look the same to the current PR. The only difference should be that the "Private" and "Withdrawn" badges are now aligned with any of the other badges.
Test the themed components by creating a themed version of them and making sure they are rendered as intended.

Checklist

• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes ESLint validation using yarn lint
• My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
• My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.

[LotteHofstede]

@atarix83 Thanks for your review!

I added the missing TypeDoc in my latest commit.

I understand your feedback about the Context being used for something very specific here and that you'd rather see it in a different type of object/enum.

Unfortunately, for the MyDSpace badges to work together with the existing badges, we needed to make MyDSpace list item components work with the listable-object-component-loader in the same way its done for all other listable components. This means that we only have access to @Inputs that are already available to us in the listable-object-component-loader. The Context enum that's already part of these @Inputs was specifically designed to still be able to differentiate between ways of rendering a listable item when all other provided data is the same.

In theory, we could adjust the listable-object-component-loader and all its related components to also use this extra @Input BadgeContext, but frankly I think this would be overkill for a value that's only used by MyDSpace list item components, especially since we already have Context to deal with specific cases.

If it would make it more readable to you, I can rename the variables instead of using a different enum.

[atarix83]

@LotteHofstede

we don't need to add the new input to all listable components, it's enough to add in the AbstractListableElementComponent that is extended by all. And to pass along by the ListableObjectComponentLoaderComponent you just need to add here the new input

For what i've seen in your code you only need to distinguish beetween the mydspace context and all the rest, is it correct ? in this case you could only provide a value for the new input in the mydspace list components only, that are a few.

maybe i'm missing something but it would not seem to require so much work, please correct me if i'm wrong

[artlowel]

@atarix83 That would still require changing quite a few components, besides I'm not fond of the idea of adding an input to something as general and widely used as listableObjectComponents for something only necessary in a handful of specific cases.

The context was designed to deal with cases where a specific listable object component needs more info about the wider context in which it's being shown. So I think @LotteHofstede is using it correctly here. Since we have nested listableObject components, you have one, general context for the outer one (workflow or workspace) and one more specific context for the inner one: the status specific ones.

You could argue that context shouldn't be a simple enum, but rather a more complex object (something you could add a mydspace status property to). But changing the type of context would require us to change literally every component using context now, so that would definitely be out of scope of this PR

[tdonohue]

@atarix83 , @artlowel , @LotteHofstede : I can see both sides to this argument, but my recommendation would be to move forward with this PR "as is" simply because a larger refactor to Context seems "out of scope" for this PR.

• Looking at where Context is used, I agree it seems to be more associated with listableObjectComponents and not specific to just search results.
• However, I agree generally with @atarix83 (and what @artlowel noted) that the approach of simply adding more entries to the Context enum seems like a "bad design" for the future. The Context enum itself seems poorly named & not well described and likely could use a refactor. But, that seems out-of-scope for this PR.

I would recommend that, in this PR, we add some TypeDocs to the Context object to describe it's purpose as it currently stands. It should describe that the enum's purpose is to provide contextual information to the @listableObjectComponent decorator, in order to allow it to render the listable object (or something like that). The current description is very vague, and it can allow for future misuse: https://github.com/DSpace/dspace-angular/blob/main/src/app/core/shared/context.model.ts#L2

I'll also give this a test/review when I get the chance, but wanted to note this immediate feedback.

[tdonohue]

@LotteHofstede and @artlowel : Tested this today, and it works well. The code overall is good... however, I agree with many of @atarix83 's minor inline comments. It'd be better to name variables badgeContext (instead of childContext) when the Context is specific to a badge. It makes the code MUCH easier to understand... as "childContext" could be misunderstood to represent a child object (like a Bundle is to an Item or similar).

I also feel that the context.model.ts file should have more inline comments / TypeDocs to better describe the use case of the Context values. So, at the least, I think the MyDSpace ones added should be described (see my inline comment for more). However, I'm OK with the current design of context.model.ts...I just think it needs more comments to make it clear.

Overall, I'm nearly a +1... just asking for (many) very minor changes.

[LotteHofstede]

@tdonohue Thanks for the feedback, I added your proposed comment and did the same for some of the other more obvious contexts.

Edit: Also added the missing contexts and changed their names

[tdonohue]

👍 Thanks @LotteHofstede ! This looks good to me. I also tested all the main features which include badges (both in "dspace" and "custom" themes): access statuses, MyDSpace, normal search, admin search. Everything looks to be working well.

I did find one very minor issue in the codebase (see inline below) which we likely should clean up before merging. I'll also wait on @atarix83 to give this one last look.

[tdonohue]

This file is missing a reference to the default SCSS in styleUrls. It looks like a default badges.component.scss file does exist (even though it's empty).

[atarix83]

I agree to not refactor the context object now. LGTM

[tdonohue]

@LotteHofstede : Can you look at the minor fix I noted above in my last review? I think it's a small bug...but if you disagree let me know. Once you have a chance to look at that and respond (or fix it), then we can merge this as it's already at +2. Thanks!

[LotteHofstede]

@tdonohue I applied the fix for your feedback!

[tdonohue]

Thanks @LotteHofstede ! Merging as this is at +2