[Task]: Limit height of some developer provided fields in reviewer tools
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
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;
}
I didn't check on this yet but accidentally I ran into this page , could be a regression.
Yes, good catch. I removed the max-width for consistency thinking the rules on the other class would be enough, but it's not...
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.
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.
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
Looks good 👍