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

Adjust the font size of the 'No thumbnails available' text. #3373

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

DanGastardelli
Copy link
Contributor

@DanGastardelli DanGastardelli commented Sep 30, 2024

References

Description

Resets the font size of the text that appears in place of thumbnails when there are no thumbnails, to prevent the text from going outside the marked thumbnail area.

Instructions for reviewers

The font size of the "No thumbnails available" text has been changed to 0.875rem at resolutions smaller than 992px and 0.4rem at very low resolutions.

List of changes in this PR:

  • Resolution treatments were added starting at line 219
    "@media screen and (max-width: map-get($grid-breakpoints, sm)) {
    font-size: 0.4rem;
    padding: 0.1rem;
    }
    @media screen and (min-width: map-get($grid-breakpoints, sm)) and (max-width: map-get($grid-breakpoints, lg)) {
    font-size: 0.875rem;
    padding: 0.1rem;
    }"
    in the file "src/styles/_global-styles.scss".

**To test the change, simply install this frontend with a clean backend, make a submission and do not run the media filter, looking at the item without a thumbnail with the text "No thumbnails available". In this scenario, test by zooming in and out on the screen and see if the text will go outside the marked area or not.

Checklist

_This checklist provides a reminder of what we will look for when reviewing your PR. You do not need to complete this checklist before creating your PR (draft PRs are always welcome).

However, reviewers may ask you to complete any actions on this list if you have not already done so. If you are unsure about an item on the checklist, please don't hesitate to ask. We are here to help!_

  • My PR is created against the main branch of code (unless it is a backport or a fix for a specific issue from an older branch).

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments and specs/tests), or I have provided reasons why this is not possible.

  • My PR passes ESLint validation using npm run lint

  • My PR introduces no circular dependencies (verified via npm run check-circ-deps)

  • My PR includes TypeDoc comments for all new (or changed) 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.

  • My PR is aligned with the Accessibility Guidelines if it makes changes to the UI. - [ ] My PR uses i18n (internationalization) keys instead of hardcoded English text, to allow for translations.

  • My PR includes details on how to test it. I have provided clear instructions to reviewers on how to successfully test this fix or feature.

  • If my PR includes new libraries/dependencies (in package.json), I have 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 have provided basic technical documentation in the PR itself. - [ ] If my PR fixes an issue ticket, I link them.

@alisaismailati
Copy link
Contributor

Hello @tdonohue , @DanGastardelli ,

I suggest considering that the "no thumbnail" placeholder font size rules are defined in https://github.com/DSpace/dspace-angular/blob/main/src/styles/_global-styles.scss#L179-L222 .

I would recommend using the mixin @include media-breakpoint instead of the media query, as the mixin provides a more concise and consistent way to handle breakpoints.

@DanGastardelli
Copy link
Contributor Author

DanGastardelli commented Oct 22, 2024

Hello @alisaismailati and @tdonohue!

I understood the requests and remade the changes. I tested it in several resolutions and it didn't break and I believe it is now applied in a better way.

I await your feedback,
Daniel Pais (Neki-it)

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.

@DanGastardelli : Thanks! I finally got around to testing this today against the main branch. While this PR is an improvement, I'm still seeing the "No thumbnails available" text breaking out of the box when the window is around 1020px wide. Here's a screenshot:
thumbnail-wrapping

While I'm not against merging this PR as-is, it might be nice to see if we can get this cleaned up as well (if you have time).

@DanGastardelli
Copy link
Contributor Author

Hi @tdonohue!

I didn't get this same scenario, even with a width of 1020px, it wasn't breaking for me; but in tests on extremely small screens, below 300px, the text was still breaking. I believe that using such a low resolution is a very specific situation, but during the analysis I saw that there was a class without correct treatment, in this case the 'thumb-font-2' class. This class appears on the home page and that's why this behavior must be happening to you. I added the treatment in this scenario and now in any resolution the text breaks for me; I believe it should work in your scenario as well.

I await feedback,
Daniel Pais (Neki-it)

@tdonohue tdonohue self-requested a review January 6, 2025 18:28
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.

@DanGastardelli : I gave this another test today. It's working to ensure that the "No thumbnails available" text never seems to break out of the box (tested on Chrome & Firefox).

However, the largest font seems to be too large. If I visit the search results with this PR, I see this (on both Firefox & Chrome). As you can see the word "Thumbnail" wraps oddly:

thumbnail-new

Instead, on our sandbox site (http://sandbox.dspace.org), the search results look better. They look like this:
thumbnail-old

So, I think this PR is moving in the right direction, but still needs some work. We should avoid wrapping text in the middle of a word. Instead, a smaller font should be used.

@DanGastardelli
Copy link
Contributor Author

Hi @tdonohue!

I was unable to achieve the same scenario as you in this resolution and I did not experience any breakage in other resolutions. However, I reduced the text to a size that is still comfortable in the scenario indicated on my machine and that can be corrected in this scenario indicated in the ticket and for others that have this same screen configuration.

I await your feedback,
Daniel Pais (Neki-it)

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 accessibility bug
Projects
Status: 👀 Under Review
Development

Successfully merging this pull request may close these issues.

Accessibility Issue with zooming in on default thumbnail images.
3 participants