report: vertically center thumbnails #11220
Conversation
| @@ -1410,6 +1411,9 @@ | |||
| width: var(--image-preview-size); | |||
| height: var(--image-preview-size); | |||
| display: block; | |||
| position: absolute; | |||
There was a problem hiding this comment.
could you try using flexbox instead?
https://css-tricks.com/snippets/css/a-guide-to-flexbox/
should be something like set display: flex; align-items: center on .lh-table-column--url
There was a problem hiding this comment.
yeah, using display: flex was my original approach but I missed using it on .lh-table-column--url because it wasn't working, I'll fix it!
There was a problem hiding this comment.
update @connorjclark so when I tried to use display: flex; align-items: center; on .lh-table-column--url or .lh-table .lh-table-column--thumbnail I wasn't getting any movement, I couldn't find the official docs for it but this post suggests that using align-items only works if we explicitly sized the height of the container which I don't think we do
When I tried to check if explicitly sizing a column entry helped, it did center the child element within the parent container, but then the row was not evenly centered, as some elements had different column heights than others
The other method I tried out did not use any flexboxes at all and only involved overriding .lh-table tr's vertical-align: baseline to be vertical-align: middle in .lh-table .lh-table-column--thumbnail. This generally worked well:
Is the vertical-align method fine or should I continue looking at flexbox methods?
There was a problem hiding this comment.
this post suggests that using align-items only works if we explicitly sized the height of the container which I don't think we do
that doesn't seem right. for example, lh-load-opportunity__col is a flex container using align-items w/o an explicit height, and it works.
I forgot that flex doesn't work to well within table elements. https://stackoverflow.com/a/41421700/2788187 .
given all this... I suppose the original PR wfm
There was a problem hiding this comment.
I don't pretend to know CSS too well, but I just nerdsniped myself and am going to read through https://www.w3.org/Style/CSS/read.en.html over the next couple weeks ..
|
the first attempt on this PR used However, when the thumbnails were larger than the other columns of information we had this behavior: The second method used The third method and preferred method so far involved replacing However, this third method vertically centers all rows in the report tables, so the following changes would also happen throughout the report: Additionally, it looks like with |
lemcardenas
added
waiting4committer
and removed
waiting4reviewer
waiting4committer
labels
* upstream/master: (42 commits) docs: add Code of Conduct to project (GoogleChrome#11212) docs(readme): add related project: lighthouse-viewer (GoogleChrome#11250) core(font-size): remove deprecated DOM.getFlattenedDocument (GoogleChrome#11248) misc: fix typo in method name (GoogleChrome#11239) i18n: make double dollar validation less strict (GoogleChrome#10299) misc: rephrase comments to be more inclusive (GoogleChrome#11228) misc: tweak gcp scripts to work in google corp (GoogleChrome#11233) v6.2.0 (GoogleChrome#11232) report: correctly display CLS in budget table (GoogleChrome#11209) report: vertically center thumbnails (GoogleChrome#11220) i18n: import (GoogleChrome#11225) tests: istanbul ignore inpage function (GoogleChrome#11229) deps(snyk): update script to prune <0.0.0 and update snapshot (GoogleChrome#11223) core(stacks): timeout stack detection (GoogleChrome#11172) core(config): unsized-images to default (GoogleChrome#11217) core(image-elements): collect CSS sizing, ShadowRoot, & position (GoogleChrome#11188) core: add FormElements gatherer (GoogleChrome#11062) new_audit: report animations not run on compositor (GoogleChrome#11105) tests: update chromestatus expecatations (GoogleChrome#11221) deps: update dot-prop secondary dependency (GoogleChrome#11198) ...
Summary
This PR addresses #11218 to vertically center thumbnails with respect to their rows in the lighthouse report.
Needed to merge #11217
previously:
with this change:
Related Issues/PRs
#11217 #11218