Add logo max constraints #2755
Add logo max constraints #2755
Conversation
tdonohue
added
bug
themes
1 APPROVAL
|
@alexandrevryghem : If you have a moment, it'd be wonderful if you could review this follow up to #2744... as it seems to provide the changes you requested. If you don't have time, I'll look for a different reviewer. But, I was hopeful it'd be a quick review for you. |
There was a problem hiding this comment.
@FrancescoMolinaro: I tested your PR but it doesn't work, I tried uploading an image with a width larger than 500px & one with a height larger than 500px. I added an inline comment with the fix, could you update it & retest it yourself.
| .logo-container { | ||
| max-width: var(--ds-comcol-logo-max-width); | ||
| max-height: var(--ds-comcol-logo-max-height); | ||
| } |
There was a problem hiding this comment.
img {
max-width: var(--ds-comcol-logo-max-width);
max-height: var(--ds-comcol-logo-max-height);
height: auto;
width: auto;
}There was a problem hiding this comment.
Hi @alexandrevryghem , thanks for your feedback.
May I ask you how you performed the tests?
I was not able to reproduce the problem on communities and collections with the current code, the image sizes result capped to 500px and I would like to know how to reproduce the error, so that I can properly check if the current or the proposed change are correctly working.
On my side I have tested the solution creating a new community or collection and uploading a very large image (7724 × 5148 pixels) and it seems fine for both.
Thanks in advance for the clarification.
There was a problem hiding this comment.
This is the test image I used:
Results in this:
There was a problem hiding this comment.
Hi @alexandrevryghem , thanks again for the feedback and the test scenario.
I ran some tests and actually the image you provided is rendered out of the container.
I have adapted the solution setting the max sizes directly on the img as you suggested, I've just omitted the auto values since they are the default and not necessary.
There was a problem hiding this comment.
Hi @FrancescoMolinaro, it's almost good. I also tested it in mobile mode today and I saw that the image overflowed out of the page this can easily be fixed by applying a width: 100%; on the img.
There was a problem hiding this comment.
Hi @alexandrevryghem , thanks again for checking, i have adapted the pull request it looks good also on mobile now:
There was a problem hiding this comment.
Thanks @FrancescoMolinaro! Everything now seems to work like expected.
|
As this is now at +2, I'm merging immediately. I'll also auto-port this to 7.6.x as the changes are only in CSS/styling which is compatible with a bug fix release. Thanks @FrancescoMolinaro ! |
|
Successfully created backport PR for |
tdonohue
removed
the
port to dspace-7_x
[DSC-2050] fix missing queryParam in url from download metric Approved-by: Francesco Molinaro Approved-by: Giuseppe Digilio
References
Fixes #2745
Description
To prevent logos to be over sized we added a configuration possibility so that on each environment we can restrict the size to a number of defined pixels.
Instructions for Reviewers
In order to test try creating a new collection or community uploading a very large picture as logo to check if it is rendered within the defined configuration sizes.
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.