addons icon indicating copy to clipboard operation
addons copied to clipboard

[Task]: Limit height of some developer provided fields in reviewer tools

Open wagnerand opened this issue 9 months ago • 1 comments

Description

In order to help with readability on the reviewer tools review page, we'd like to limit the height of the following fields:

  • Version Notes
  • Notes for Reviewers
  • Reviewer reply

A max height of 50em seems reasonable.

We want to make sure though that it's apparent to the reader when there is more text than is visible in a field. Using overflow-y: scroll was suggested, but that doesn't seem to work on macOS at least, the scrollbars are still not shown (example) and this might be a macOS-specific setting or override. I am not sure how other operating systems behave.

Acceptance Criteria

  • [ ] Limit the height of some fields in reviewer notes (see above)
  • [ ] Make it apparent to reviewers when a field has more content than is visible

┆Issue is synchronized with this Jira Task

wagnerand avatar Mar 27 '25 15:03 wagnerand

As noted, by default browsers might not always show the scrollbar even with overflow: scroll. This is an old problem, but fortunately there is a clever hack that should work to make the scrollable box stand out: https://lea.verou.me/blog/2012/04/background-attachment-local/

Something like this should work (I think that would cover developer replies, reviewer replies, release notes and notes for reviewers, need to double-check):

.history-notes, .history-comment {
    overflow-y: scroll;
    max-height: 50em;
    background:
        /* Shadow covers */
        linear-gradient(white 30%, rgba(255,255,255,0)),
        linear-gradient(rgba(255,255,255,0), white 70%) 0 100%,
        
        /* Shadows */
        radial-gradient(50% 0, farthest-side, rgba(0,0,0,.2), rgba(0,0,0,0)),
        radial-gradient(50% 100%,farthest-side, rgba(0,0,0,.2), rgba(0,0,0,0)) 0 100%;
    background:
        /* Shadow covers */
        linear-gradient(white 30%, rgba(255,255,255,0)),
        linear-gradient(rgba(255,255,255,0), white 70%) 0 100%,
        
        /* Shadows */
        radial-gradient(farthest-side at 50% 0, rgba(0,0,0,.2), rgba(0,0,0,0)),
        radial-gradient(farthest-side at 50% 100%, rgba(0,0,0,.2), rgba(0,0,0,0)) 0 100%;
    background-repeat: no-repeat;
    background-color: white;
    background-size: 100% 40px, 100% 40px, 100% 14px, 100% 14px;
    background-attachment: local, local, scroll, scroll;
}

diox avatar Mar 27 '25 18:03 diox

I didn't check on this yet but accidentally I ran into this page , could be a regression.

ioanarusiczki avatar Apr 08 '25 09:04 ioanarusiczki

Yes, good catch. I removed the max-width for consistency thinking the rules on the other class would be enough, but it's not...

diox avatar Apr 08 '25 09:04 diox

I think that broken layout has to to with a long unbroken string, if you look at it Version 4.0 it has that.

I reproduced the issue on dev with this addon , then with a long text it looks good.

ioanarusiczki avatar Apr 09 '25 09:04 ioanarusiczki

Yes, I have a fix (hopefully) that I just merged in https://github.com/mozilla/addons-server/commit/fadb56aec569be27300109977c8e7866f84bc1e8 ; it should land on dev shortly.

diox avatar Apr 09 '25 09:04 diox

Ah yes, I see it's a regression actually because on dev it breaks the layout while on stage it won't. this is now dev https://reviewers.addons-dev.allizom.org/en-US/reviewers/review-listed/636797#review-actions this is now stage https://reviewers.addons.allizom.org/en-US/reviewers/review-listed/2246741#review-actions

ioanarusiczki avatar Apr 09 '25 09:04 ioanarusiczki

Looks good 👍

ioanarusiczki avatar Apr 09 '25 12:04 ioanarusiczki