-
Notifications
You must be signed in to change notification settings - Fork 437
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
Conversation
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 @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> |
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.
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
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.
Thank you @artlowel we weren't aware of this re-render issue. We will do as you suggest. Thank you for your feedback!
@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). |
.../shared/object-collection/shared/badges/access-status-badge/access-status-badge.component.ts
Outdated
Show resolved
Hide resolved
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 @paulo-graca!
Successfully created backport PR for |
[DSC-2052] change canDownload method with pipe Approved-by: Giuseppe Digilio
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:
and for Items and Publications (if you use Entities), inspect the access status value, like: "Open Access" and you should see something like:
List of changes in this PR: