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

Maintain a two-column layout on desktop screen sizes #8248

Closed
1 task
johanstromqvist opened this issue Feb 2, 2023 · 13 comments
Closed
1 task

Maintain a two-column layout on desktop screen sizes #8248

johanstromqvist opened this issue Feb 2, 2023 · 13 comments

Comments

@johanstromqvist
Copy link
Contributor

johanstromqvist commented Feb 2, 2023

Issue following the viewport discussion in #7966

Summary

  • The page width, nav and ToC are causing the content column to be narrow
  • The narrow viewport width often causes polaris small breakpoint to trigger inside it
  • The small breakpoint shows single column layouts, which is different from what images, content and rationale demonstrate
  • The viewport height sometimes cause rendered elements to collapse vertically, which is uncommon in product and not good for demonstration purposes Height related issues moved to #8345

Eng tasks

@johanstromqvist
Copy link
Contributor Author

johanstromqvist commented Feb 2, 2023

Snippets from #7966

Short term
Go with what's easiest to build and has the least implications on the general page experience. Our bet is that this is the zoomable iframe solution. This approach is hacky and does come with UX tradeoffs. We think we can mitigate and accept some of those in the short term, given that we revisit the purpose and design of the site/page/code feature later.

Long term
Revisit this problem when we have learned more about different pattern needs in general and the code feature specifically. When we do revisit it, we should probably address the width of the content column, which is a site-wide root problem, and take a critical look at the nav and ToC. The Tailwind Awesome demo is an amazing reference, and imo the most interesting direction. It's just a matter of timing.

Ideas:

Tailwind Awesome inspired
image

iFrame zoom
image

Overflow
image

@johanstromqvist
Copy link
Contributor Author

Approach 1

  • Reduce site-wide padding to 1.25rem/20px and gap to 2.5rem/40px
  • These are the same values as shopify.dev
  • This provides enough space for md breakpoint to remain unchanged from 1098px window width and up, including when ToC appears

Approach 2

  • Keep padding and gap as is, but don't let ToC appear until there is space for both md breakpoint and ToC
  • This would increase the doc site breakpoint from 1400px to 1550px

Reference

On shopify.dev:

  • the ToC width is 277px
  • the padding around both content column and ToC is 20px
  • the max column width is 911px

image

image

@johanstromqvist
Copy link
Contributor Author

johanstromqvist commented Feb 3, 2023

Design proposal (approach 3)

  • Site-wide reduction of padding 1.25rem and gap 2.5rem (matches shopify.dev)
  • Site-wide max width to content column 776px

Rationale

  • More effective space usage on small screen sizes, and same space usage on large screen sizes
  • The viewport triggers md breakpoint sooner; at 1098px instead of 1186px
  • Doc site breakpoint don't shrink the content width when the ToC appears
  • The md breakpoint persists from 1098px window width and up, instead of being reduced to s at 1400px

Before

After

Changes in prototype
image

@johanstromqvist johanstromqvist changed the title Update dimensions and behaviour of code render viewport Maintain a two-column layout on larger screen sizes Feb 3, 2023
@johanstromqvist johanstromqvist removed their assignment Feb 3, 2023
@johanstromqvist johanstromqvist changed the title Maintain a two-column layout on larger screen sizes Maintain a two-column layout on desktop screen sizes Feb 3, 2023
@johanstromqvist
Copy link
Contributor Author

@jesstelford & @gwyneplaine, please check out the suggestion above and let me know what you think.

The outstanding question is regarding viewport height. How is that currently set? Any thoughts on how it should be set? Two observations on height related issues this far:

  1. The range picker gets scrollable overflow due to low viewport height
  2. Layout viewports can get quite tall (which might not be a problem at all)

@jesstelford
Copy link
Contributor

Site-wide padding change is a great option: Ticks most of the boxes with minimal effort 👍

How is [viewport height] currently set?

On initial load, the iframe is resized to be 100% height of the rendered example. The height then stays the same until the page is reloaded. This means if you resize your browser, the height will not change to accomodate the new height of the example. There's also an edge case where if content is rendered after initial load (eg; clicking to open a modal, etc) which changes the height, the iframe will not be resized.

We might be able to cook up something to detect changes to the page width and update the height as the browser is resized, however this wont fix the edge case where rendering after initial load changes the height. I'm not sure how to handle that 🤔

@johanstromqvist
Copy link
Contributor Author

Moved height related issues to #8345 (backlog)

@SeanyB
Copy link
Contributor

SeanyB commented Feb 14, 2023

@gwyneplaine - I see this moved into Ready for review, but there aren't changes on staging yet. Is #8338 the PR that adds the adjustments to left and right padding and TOC behaviour?

@johanstromqvist
Copy link
Contributor Author

johanstromqvist commented Feb 15, 2023

Seems to be live on staging now 👍

These two fixes look great:

  • Updated Page styles to reduce gap from 5rem to 2.5rem
  • Update Container styles to reduce horizontal padding from 4rem to 1.5rem

Another proposed change was to limit the max-width to 776px and justify the content center at breakpoints below 1400px. Were those changes missed or discussed somewhere? AFAIK they might still be needed, sorry if they were unclear.

Context

Currently, the content width only surpasses 776px at $breakpointDesktopSmall before the ToC appears. Beyond that breakpoint the max width is 776px. As the content width increases between 777px and 1399px window widths, the content width causes line lengths to become very long and images to be blown up in scale.

Image

Image

@jesstelford
Copy link
Contributor

Good catch! Fixed in #8399

@johanstromqvist
Copy link
Contributor Author

johanstromqvist commented Feb 16, 2023

Sorry that I didn't think ahead and make this clear earlier, but the width cap fix was intended for the "editorial" page type. That's where we come across the issues with line lengths and massive images. As is, #8399 also caps the width of index pages and "sell pages", which leads to ineffective use of space and some odd looks.

Pages the width cap changes shouldn't apply to:

(I thiiink this is a complete list)

👇 Examples of pages that should use the available width (with whatever our current logic is)

💁 Yay 🙅 Nay
image image
image image
image image

@jesstelford
Copy link
Contributor

jesstelford commented Feb 19, 2023

Ah, ok, thanks for clarifying!

Here's a PR with a fix: #8432

@jesstelford
Copy link
Contributor

jesstelford commented Feb 20, 2023

Separate to the fixed width column, the changed page padding means tokens now have enough space to show 4 per row @ 1400px:

Prod Patterns branch
tokens-before tokens-after

Is this ok? Or should we make it a fixed width column too?

@johanstromqvist
Copy link
Contributor Author

Yes, I think that's a good use of space in general.

However, the tokens index switch to a table view from at >1400, so the four items per row is only effective between 1396px and 1400px. I don't see any big concerns with this, but I thought I'd call it out in case there's any messy breakpoints leading to that.

image

image

image

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

No branches or pull requests

3 participants