Set fixed min-height on main page #2942

jimbo · 2021-01-12T23:29:32Z

Description

The Main component, which renders the header, page, and footer, sets min-height: 100vh on the page. As intended, this style prevents the footer from ever appearing above the fold. Unfortunately, setting a height with relative units causes issues when the page is rendered in an <iframe>, so we'll have to remove it.

This PR changes to min-height: 40rem (640px) and addresses any fallout from that change.

Related Issue

PWA-1243.

Acceptance

Verification Stakeholders

Specification

Verification Steps

Visit the home page.
Verify that the footer is at least 640px below the header.
Repeat for /cart, /checkout, and other pages.
On an empty page or 404 page, ensure the footer is only 640px below the header.

Screenshots / Screen Captures (if appropriate)

Checklist

I have added tests to cover my changes, if necessary.
I have added translations for new strings, if necessary.
I have updated the documentation accordingly, if necessary.

PWAStudioBot · 2021-01-12T23:39:32Z

	Messages
📖	Associated JIRA tickets: PWA-1243.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 7ac84f1

sirugh

Going to need to re-think the global padding. Also, something seems off with the page width and the left nav height:

sirugh · 2021-01-13T15:42:37Z

packages/venia-ui/lib/components/Main/main.css

@@ -12,7 +12,8 @@
 .page {
    margin: 0 auto;
    max-width: var(--venia-global-maxWidth);
-    min-height: 100vh;
+    min-height: 40rem;


This is much more palatable than 100vh :) Nice and simple.

sirugh · 2021-01-13T15:42:56Z

packages/venia-ui/lib/components/Main/main.css

@@ -12,7 +12,8 @@
 .page {
    margin: 0 auto;
    max-width: var(--venia-global-maxWidth);
-    min-height: 100vh;
+    min-height: 40rem;
+    padding: 1rem 0 1.5rem;


I don't think this will work - some components are meant to fit snug against the header ie error page and the home page sample content. Notice the gap:

fooman · 2021-01-13T21:34:52Z

Bringing back the footer into view does cause quite a bit of jumping around on any pages that load content async like product and categories - https://pr-2942.pwa-venia.com/venia-tops/venia-blouses.html?page=1 which will affect Cumulative Layout Shift negatively https://web.dev/cls/

jimbo · 2021-01-13T22:26:11Z

Bringing back the footer into view does cause quite a bit of jumping around on any pages that load content async like product and categories - https://pr-2942.pwa-venia.com/venia-tops/venia-blouses.html?page=1 which will affect Cumulative Layout Shift negatively https://web.dev/cls/

Yes, very good point. This highlights how problematic it is to async-load content without placeholders that allow layout to pre-allocate space for that content. Lighthouse does and should call this out, and we do need to address it.

brendanfalkowski · 2021-01-13T23:06:32Z

Devil's advocate though — loading skeletons almost always have CLS too. They just never seem to be content-shaped enough to perfectly avoid it. Yeah Google wants you to avoid it, but can anyone without SSR? This just feels like one of those "a real project would never allocate resources to fixing it when the backlog has 200 tickets" areas.

sirugh · 2021-01-14T14:55:51Z

packages/venia-ui/lib/components/Main/main.css

@@ -10,9 +10,10 @@
 }

 .page {
+    display: grid;


Causes issues on mobile for the CMS page. Also for cart page as the grid aligns center but then we have extra padding on the page itself. Do we need this attribute?

jimbo · 2021-01-25T18:13:21Z

Devil's advocate though — loading skeletons almost always have CLS too. They just never seem to be content-shaped enough to perfectly avoid it. Yeah Google wants you to avoid it, but can anyone without SSR? This just feels like one of those "a real project would never allocate resources to fixing it when the backlog has 200 tickets" areas.

That's true, but CLS measures how much layout shift, so even an imperfect skeleton is better than none.

That said, it's also true that layout shift will continue to be a problem until we land SSR. Because routing information is stored on the server rather than the client, we can't even render a meaningful skeleton until useMagentoRoute finishes its query, at which point rendering a skeleton is questionable.

Accordingly, I've stripped this PR down to the essential bugfix and nothing more.

Removed the offending line.

dpatil-magento · 2021-02-01T18:48:02Z

@jimbo Looks good. Just a query when page content is very less, should there be max height?

jimbo · 2021-02-02T17:39:53Z

@jimbo Looks good. Just a query when page content is very less, should there be max height?

Should be fine as it is.

dpatil-magento · 2021-02-02T18:25:04Z

QA Approved.

Set fixed min-height on main page

bd7c408

jimbo added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Jan 12, 2021

m2-community-project bot added Progress: ready for review labels Jan 12, 2021

PWAStudioBot added the pkg:venia-ui label Jan 12, 2021

sirugh previously requested changes Jan 13, 2021

View reviewed changes

m2-community-project bot assigned sirugh Jan 13, 2021

m2-community-project bot added Progress: changes requested and removed Progress: ready for review labels Jan 13, 2021

Collapse margins without padding

f31a96b

sirugh reviewed Jan 14, 2021

View reviewed changes

m2-community-project bot added Progress: review and removed Progress: changes requested labels Jan 14, 2021

jimbo added 2 commits January 20, 2021 13:51

Adjust min-height

0c8b0b3

Revert grid to block

b31fca9

supernova-at approved these changes Jan 28, 2021

View reviewed changes

m2-community-project bot assigned supernova-at Jan 28, 2021

m2-community-project bot added Progress: approved and removed Progress: review labels Jan 28, 2021

Merge branch 'develop' into jimbo/min-height

fdd39ad

sirugh approved these changes Jan 29, 2021

View reviewed changes

Merge branch 'develop' into jimbo/min-height

7ac84f1

dpatil-magento merged commit d73de18 into develop Feb 2, 2021

dpatil-magento deleted the jimbo/min-height branch February 2, 2021 18:25

m2-community-project bot added Progress: done and removed Progress: approved labels Feb 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set fixed min-height on main page #2942

Set fixed min-height on main page #2942

jimbo commented Jan 12, 2021

PWAStudioBot commented Jan 12, 2021 •

edited

Loading

sirugh left a comment •

edited

Loading

sirugh Jan 13, 2021

sirugh Jan 13, 2021

fooman commented Jan 13, 2021

jimbo commented Jan 13, 2021

brendanfalkowski commented Jan 13, 2021

sirugh Jan 14, 2021 •

edited

Loading

jimbo commented Jan 25, 2021

dpatil-magento commented Feb 1, 2021

jimbo commented Feb 2, 2021

dpatil-magento commented Feb 2, 2021

Set fixed min-height on main page #2942

Set fixed min-height on main page #2942

Conversation

jimbo commented Jan 12, 2021

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Jan 12, 2021 • edited Loading

sirugh left a comment • edited Loading

Choose a reason for hiding this comment

sirugh Jan 13, 2021

Choose a reason for hiding this comment

sirugh Jan 13, 2021

Choose a reason for hiding this comment

fooman commented Jan 13, 2021

jimbo commented Jan 13, 2021

brendanfalkowski commented Jan 13, 2021

sirugh Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

jimbo commented Jan 25, 2021

dpatil-magento commented Feb 1, 2021

jimbo commented Feb 2, 2021

dpatil-magento commented Feb 2, 2021

PWAStudioBot commented Jan 12, 2021 •

edited

Loading

sirugh left a comment •

edited

Loading

sirugh Jan 14, 2021 •

edited

Loading