Skip to content
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 new access-status-list-element-badge css classes #2579

Merged
merged 5 commits into from
Nov 13, 2023

Conversation

paulo-graca
Copy link
Contributor

References

Description

This PR adds CSS classes to access-status-badge component in order to make possible to differentiate access status based on the value itself.

Instructions for Reviewers

In order to test it you need to activate in your angular configuration:

# Item Config
item:
  showAccessStatuses: true

and for Items and Publications (if you use Entities), inspect the access status value, like: "Open Access" and you should see something like:

<span class="badge badge-secondary access-status-list-element-badge access-status-open-access-listelement-badge">Open Access</span>

List of changes in this PR:

  • We just changed the HTML part of the component and replace "." with "-" of the accessStatus value.

@paulo-graca paulo-graca added the 1 APPROVAL pull request only requires a single approval to merge label Oct 26, 2023
@paulo-graca paulo-graca self-assigned this Oct 26, 2023
@tdonohue tdonohue added bug port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release labels Oct 26, 2023
@tdonohue tdonohue requested a review from artlowel October 26, 2023 14:43
Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @paulo-graca

It works, but I just have an inline comment about the performance.

It would also be good to already provide default colors for the access statuses. Note that you can reuse bootstrap's own badge mixin for that e.g.:

.access-status-open-access-listelement-badge {
  @include badge-variant(map-get($theme-colors, warning));
}

this would make the access-status-open-access-listelement-badge class idential to the badge-warning class (i.e. orange)

@@ -1,5 +1,5 @@
<ng-container *ngIf="showAccessStatus">
<span *ngIf="accessStatus$ | async as accessStatus">
<span class="badge badge-secondary">{{ accessStatus | translate }}</span>
<span [class]="'badge badge-secondary access-status-list-element-badge ' + accessStatus.replaceAll('.','-')">{{ accessStatus | translate }}</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For performance reasons, it's not a good idea to put function calls in a template. Angular will re-render templates often, sometimes multiple times per second, so the exact same replace will happen many more times than necessary.

It's better to do the replace in the the map in the component ts file and map it to an object with a property for the label and the class (which is the label with the replacements). That way the replace only happens once

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @artlowel we weren't aware of this re-render issue. We will do as you suggest. Thank you for your feedback!

@paulo-graca
Copy link
Contributor Author

paulo-graca commented Nov 10, 2023

@artlowel I think I've changed it in the way you suggested. I've also added an empty scss file, but without specifying anything. I think we all don't have a clear idea what access status should look like and you can confirm that on the issue discussion page (#2402).
We are currently working in a custom theme that will address this, when finished, if all agree, we can proposed it to be the default. It will be something like:
image
and
image

@paulo-graca paulo-graca requested a review from artlowel November 10, 2023 00:20
@paulo-graca paulo-graca requested a review from artlowel November 10, 2023 15:19
Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @paulo-graca!

@artlowel
Copy link
Member

We are currently working in a custom theme that will address this, when finished, if all agree, we can proposed it to be the default. It will be something like:
image
and
image

I like the way that looks

@tdonohue tdonohue merged commit 55435a2 into DSpace:main Nov 13, 2023
10 checks passed
@dspace-bot
Copy link
Contributor

Successfully created backport PR for dspace-7_x:

@tdonohue tdonohue removed the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label Nov 13, 2023
@tdonohue tdonohue added this to the 8.0 milestone Nov 13, 2023
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Nov 27, 2024
[DSC-2052] change canDownload method with pipe

Approved-by: Giuseppe Digilio
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge bug
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Visual differentiation needed for Access Status
4 participants