Skip to content

Conversation

@spaceninja
Copy link
Member

@spaceninja spaceninja commented Nov 18, 2021

Overview

This PR fixes a bug where blog posts that contained lengthy code samples were breaking layout. The bug wasn't actually caused by the code samples, which have the proper overflow:auto property set. Instead, this is caused by an oddity of Grid layouts, where the Grid items by default have min-width:auto, which can prevent them from reducing width the way you might expect. The fix is to tell the Grid layout the minimum width for Grid items, and then it will shrink the way you'd expect.

Learn more: https://css-tricks.com/you-want-minmax10px-1fr-not-1fr/

Testing

Unfortunately, the bug was not visible in the pattern library, because we weren't applying the Page object to the Single Article prototype. This PR applies that change so we can test the fix.

  1. visit the Single Article prototype on the deploy preview
  2. Using devtools, disable the grid-template-columns rule on .o-page.
  3. Reduce your browser to small screen size, and you should see the layout break, with a horizontal scrollbar on the entire page.
  4. Restore the grid-template-columns rule.
  5. The layout should now be fixes.

This PR fixes a bug where blog posts that contained lengthy code
samples were breaking layout. The bug wasn't actually caused by the
code samples, which have the proper `overflow:auto` property set.
Instead, this is caused by an oddity of Grid layouts, where the Grid
items by default have `min-width:auto`, which can prevent them
from reducing width the way you might expect. The fix is to tell
the Grid layout that the minimum width for Grid items is 0, and then
it will shrink the way you'd expect.

@see https://ishadeed.com/article/min-content-size-css-grid/

Fixes #1532
@spaceninja spaceninja requested review from a team November 18, 2021 23:40
@changeset-bot
Copy link

changeset-bot bot commented Nov 18, 2021

⚠️ No Changeset found

Latest commit: 86b1791

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented Nov 18, 2021

✔️ Deploy Preview for cloudfour-patterns ready!

🔨 Explore the source changes: 86b1791

🔍 Inspect the deploy log: https://app.netlify.com/sites/cloudfour-patterns/deploys/61981ac6b7631d0007099cde

😎 Browse the preview: https://deploy-preview-1591--cloudfour-patterns.netlify.app

Comment on lines 30 to 35
min-width: 0; // 1
}

.o-page__footer {
grid-row: 2;
min-width: 0; // 1
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, would setting grid-template-columns: minmax(0, 1fr); on the o-page element have the same effect as setting min-width: 0 on the child elements?

https://css-tricks.com/you-want-minmax10px-1fr-not-1fr/

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I tried out @tylersticka's suggestion in the browser via DevTools and it seems to work. I'm am going to read the article to better understand why. 📖 🔍 👀

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I'm convinced in that we should explore grid-template-columns: minmax(0, 1fr); as @tylersticka suggested! 🙂

This article helped explain a bit more for me (linked in the original shared article): https://css-tricks.com/preventing-a-grid-blowout/

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, thanks @tylersticka!

Updated.

Copy link
Member

@gerardo-rodriguez gerardo-rodriguez left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@spaceninja spaceninja merged commit eab6bf4 into v-next Nov 19, 2021
@spaceninja spaceninja deleted the bugfix/grid-min-width branch November 19, 2021 22:07
spaceninja added a commit that referenced this pull request Nov 22, 2021
* v-next:
  Improve themes toolbar, add text flow toolbar (#1592)
  Update dependency @types/jest to v27.0.3
  Update dependency @babel/preset-env to v7.16.4
  Add min-width to Page contents (#1591)
  Remove paddings add-on (#1590)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Some blog posts are extending past their containers

4 participants