Skip to content

adding new access-status-list-element-badge css classes#2579

Merged
tdonohue merged 5 commits intoDSpace:mainfrom
paulo-graca:bugfix/accessstatus-2402
Nov 13, 2023
Merged

adding new access-status-list-element-badge css classes#2579
tdonohue merged 5 commits intoDSpace:mainfrom
paulo-graca:bugfix/accessstatus-2402

Conversation

@paulo-graca
Copy link
Copy Markdown
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
Copy Markdown
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)

<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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@dspace-bot
Copy link
Copy Markdown
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