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

clients(devtools): fix collapsing-width svg in flexbox #9602

Merged
merged 1 commit into from
Aug 27, 2019
Merged

Conversation

brendankenny
Copy link
Member

@exterkamp discovered that the stack pack logos are hidden (sized 0x0) in the report in devtools, then I noticed that devtools has a * {min-width: 0} rule applied to everything which, seemingly improbably, is causing this.


Not a flexbox expert, so take the following with a grain of salt:

It turns out this is acting correctly. Flexbox uses an element's dimensions for sizing, and if an SVG doesn't have an explicit size (the wordpress logo has a viewBox, but not a width/height) it falls back to min-width. And since that's 0 in devtools, it collapses the SVG to 0x0.

If we instead set it to min-width: auto, a default width of 300px is used, which is then constrained by the max-width: 50px. This appears to be what happens in the non-devtools report.

for more, read CSS Box Sizing : Intrinsic Sizes (under For boxes with an intrinsic aspect ratio, but no intrinsic size).


We could give this particular svg a width/height or give .lh-audit__stackpack__img a min-width: auto, but it seems like this could just come back to bite us again (there may in fact be other svg in the report we aren't noticing is missing).

Instead, since this seems limited to svg images in particular, it seems ok to reset min-width for all images within the lighthouse report, leaving min-width: 0 to do whatever it's supposed to be doing in devtools everywhere else.

@brendankenny brendankenny changed the title clients(devtools) fix collapsing-width svg in flexbox clients(devtools): fix collapsing-width svg in flexbox Aug 23, 2019
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like a compelling story to me! :)

@exterkamp
Copy link
Member

exterkamp commented Aug 24, 2019

If we had a zeit deploy that included a stack pack would that help catch this kind of thing? 😮 TIL we did that. Y'all smart

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants