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

src/app: adjust item count style #2316

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Conversation

alanorth
Copy link
Contributor

References

Description

Minor adjustment to community and collection item counts. Instead of using the Bootstrap lead class, which reduces weight but increases size, we should use the font-weight-light class. This reduces the weight alone.

See: https://getbootstrap.com/docs/4.6/utilities/text/#font-weight-and-italics

Before (in my custom theme, but the effect is the same in the default theme):

After:

Instructions for Reviewers

List of changes in this PR:

  • Change the CSS class used in the community and collection counts

Reviewers should build the frontend and make sure the style of the item counts looks OK.

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.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@alanorth alanorth added the quick win Pull request is small in size & should be easy to review and/or merge label Jun 16, 2023
@alanorth alanorth added this to the 7.6 milestone Jun 16, 2023
@tdonohue tdonohue self-requested a review June 16, 2023 14:11
@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge label Jun 16, 2023
@alanorth
Copy link
Contributor Author

@damian-joz can you give this a quick test and share your feedback so we can hopefully merge it for DSpace 7.6? Thanks!

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@alanorth : This works, but it looks like you forgot to update the http://localhost:4000/community-list page as well? That page also shows these item counts.

I think you need to make the same change here:
https://github.com/DSpace/dspace-angular/blob/main/src/app/community-list-page/community-list/community-list.component.html#L40

If you could make this change on Monday, I'll make sure to re-test it on Tues (I'm out Mon) and get it merged then.

@alanorth
Copy link
Contributor Author

@alanorth : This works, but it looks like you forgot to update the http://localhost:4000/community-list page as well? That page also shows these item counts.

Oh, good catch! Fixed and force pushed an updated commit.

@alexandrevryghem
Copy link
Member

To make the seperation between the item count & the title more clear we could maybe use the same layout as used for the item count in BrowseEntryListElementComponent (see here) and use badge badge-pill badge-secondary

@alanorth
Copy link
Contributor Author

alanorth commented Jun 18, 2023

I like the current style because it looks like the well-established DSpace 5/6 XMLUI style, but you have a point that we are already using the pill style to show counts of things in DSpace 7 so perhaps we should be consistent...?

@alanorth
Copy link
Contributor Author

alanorth commented Jun 19, 2023

Testing with @alexandrevryghem's suggestion to use Bootstrap badge pills:

It looks OK, but the vertical alignment is slightly off. Not sure if that is easy to fix without converting this to a CSS Flex / grid like in the browse subject page (see example on demo7.dspace.org).

@alexandrevryghem
Copy link
Member

A quick fix is to fix it using vertical-align, but it might indeed be better to use flex instead 🤷‍♂️

.badge {
  vertical-align: text-top;
}

@alanorth
Copy link
Contributor Author

@alexandrevryghem here is an implementation that copies the Flex one in BrowseEntryListElementComponent (in my custom theme, but you get the idea):

It's a bit too much padding (Bootstrap pr-2) on the community/collection name for my taste, but it uses the exact same implementation as on the browse by pages so it's consistent.

Let's see when @tdonohue gets back which he prefers.

@tdonohue
Copy link
Member

@alanorth and @alexandrevryghem : I'm OK with the "pill" approach since we are already using in it in "Browse by Subject" / "Browse by Author" to list the count of the number of items under each. It does keep the UI more consistent.

That said though, I'd like to get this change merged quickly... the release is likely to be cut on Thurs (June 22). So, if someone could update this PR or create a new one, I'd appreciate it.

@alanorth
Copy link
Contributor Author

@tdonohue sure, I agree that the pills are more consistent. I updated this PR to make the item counts use the same flexbox style as the "browse by" pills for the home page and sub-community pages, with a minor difference in the CSS classes on the community list.

Home page:

Sub-community page:

Community list:

P.S. does anyone else think there is a bit too much whitespace between the community/collection name and the pills?

@alanorth alanorth changed the title src/app: adjust font weight in item counts src/app: adjust item count style Jun 20, 2023
@tdonohue tdonohue self-requested a review June 20, 2023 19:14
Minor adjustment to community and collection item counts. Instead of
using the Bootstrap `lead` class, which reduces weight but increases
size, we should use the same badge / pill style used in other counts
like on on the browse by pages.
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @alanorth ! This looks good to me. I do agree there might be minor tweaks we could make in the future regarding spacing on all the pages that use these pills... but this looks good enough!

@tdonohue tdonohue merged commit ee3e5ca into DSpace:main Jun 20, 2023
10 checks passed
@alanorth alanorth deleted the item-count-style branch June 21, 2023 05:38
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 quick win Pull request is small in size & should be easy to review and/or merge
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants