MiniCart Height Fix

[supernova-at]

Description

This PR updates the measurement the MiniCart uses to compute its height to use window.innerHeight instead of 100vh as described by https://css-tricks.com/the-trick-to-viewport-units-on-mobile/.

As described in the attached issue #1071, 100vh often doesn't account for browser address bars or other frames.

This PR takes advantage of the new useWindowSize hook to update this value even after resize events occur.

Related Issue

Closes #1071 .

Verification Steps

1. Add an item to your cart
2. Open the MiniCart (right drawer)
3. See that the Checkout button is fully visible
4. Resize the window (or change orientation of device) and ensure Checkout button is fully visible
5. The payments form depended a bit on these CSS variables, so ensure that looks good too

Run the verification steps above on a variety of phones.

How Have YOU Tested this?

1. Tested using iPhone XR on iOS simulator
2. yarn test.

Screenshots / Screen Captures (if appropriate):

Before Fix

After Fix

Proposed Labels for Change Type/Package

• major (e.g x.0.0 - a breaking change)
• minor (e.g 0.x.0 - a backwards compatible addition)
• patch (e.g 0.0.x - a bug fix)

Checklist:

• I have updated the documentation accordingly, if necessary.
• I have added tests to cover my changes, if necessary.

[vercel]

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://venia-git-supernova-1071croppedcheckout2.magento-research1.now.sh

[PWAStudioBot]

	Messages
📖	We are currently working on automating the PR metadata checks. Until that time, you may see failures related to labels/description/linked issues/etc even if you have fixed the problem. Failures will persist until the next push (assuming they are fixed).

Generated by 🚫 dangerJS against b420f2b

[sirugh]

One comment, otherwise this looks good!

My only concern is that we are using resize events to update a css property which means repaint. This may cause a resize to stutter. Perhaps this is a good time to try to debounce so that we update the size on the last value after of a bunch of resize events.

[jimbo]

So my major concern with introducing hooks such as useWindowSize is that they'll be used because they're expedient, rather than as an absolute last resort. In this case, it would be more ideal and much faster for CSS to handle the sizing on its own.

Is there literally any other way to set a fixed element to be full height that does respect browser chrome? I can be the person to experiment with that, if necessary.

[supernova-at]

it would be more ideal and much faster for CSS to handle the sizing on its own.

Totally agree.

Is there literally any other way to set a fixed element to be full height that does respect browser chrome? I can be the person to experiment with that, if necessary.

Based on the CSS Tricks article the problem is that the mobile browsers have calculated 100vh to be based on the maximum height of the screen (not including any browser chrome). If we can find a way to calc that value minus the browser chrome height or something, that would definitely be ideal.

I'm not aware of any property like that in CSS though, so I went with JS window.innerHeight instead.

[supernova-at]

My only concern is that we are using resize events to update a css property which means repaint. This may cause a resize to stutter. Perhaps this is a good time to try to debounce so that we update the size on the last value after of a bunch of resize events.

Great idea but I don't actually know where we'd do that.

Here are the relevant bits:

    const { innerHeight: viewportHeight } = useWindowSize();
    const rootStyle = {
        '--minicart-height-unit': `${viewportHeight * 0.01}px`
    };

    // then...
    <aside className={rootClass} style={rootStyle}>

I don't think we can wrap debounce around useWindowSize because of the "consistent number of hook calls" restriction, and I thought we decided against adding it inside of useWindowSize itself.

Where were you thinking?

[sirugh]

I don't think we can wrap debounce around useWindowSize because of the "consistent number of hook calls" restriction, and I thought we decided against adding it inside of useWindowSize itself.

We can't debounce the hook but we can debounce the update of rootStyle:

const { innerHeight } = useWindowSize();
const [debouncedHeight, setDebouncedHeight] = useState(window.innerHeight);

useEffect(() => {
  _.debounce(() => { // obvs we could write our own or use lodash.
    setDebouncedHeight(innerHeight)
  }, 50);
}, [innerHeight]);


const rootStyle = {
  '--minicart-height-unit: `${debouncedHeight * 0.01}px`'
}
return (
  <aside className={rootClass} style={rootStyle}>
    //...
  </aside>
)

That's just rough code but the idea applies I think.

[sirugh]

I'm not aware of any property like that in CSS though, so I went with JS window.innerHeight instead.

fill-available may do the trick but it is experimental.

[supernova-at]

PR Updated:

• Debounces repaint operations (2e67196).

This one was interesting! Turns out timing events (used by lodash.debounce) have unintuitive interactions with React Hooks.

Here's some great reading: https://overreacted.io/making-setinterval-declarative-with-react-hooks/.

The code is therefore not quite as straightforward as we'd like but I added some comments to mitigate confusion.

It works great!

[supernova-at]

This change has nothing to do with this PR but it failed linting and I couldn't push without it.

[sirugh]

cough --no-verify cough I mean what? Good job thanks!

[supernova-at]

I didn't add a comment in the code here but just FYI we also had to make the debounced function a ref because otherwise a new debounced function would get created each time through this function and they would ALL fire after the debounce delay.

[sirugh]

Can we not just useMemo here?

[supernova-at]

Good question - I had that thought too but for some reason tried useRef first. Since it worked I didn't try useMemo.

If you think it's better I can switch it.

[sirugh]

Isn't this also going to multiple references to setViewportHeightDebounced and only invoke one? As in whichever is attached to the ref last? It just feels overly confusing currently but I can see now how my original example would have queued a bunch of useEffect callbacks too...

[coveralls]

Coverage increased (+0.03%) to 77.659% when pulling deaa1bd on supernova/1071_cropped_checkout_2 into 58184db on develop.

[jimbo]

@supernova-at That's a great article from Dan. I would have expected us to debounce things inside useWindowSize, though, since that's where the changes are published. Updating the context provider on every resize event is probably too noisy, so if we debounce there, we won't have to do a bunch of ref work in all of the consumers.

Also, I think height: 100% will work, no JS sizing necessary. Try it out in XCode's simulator, it works.

[sirugh]

I would have expected us to debounce things inside useWindowSize

Didn't we talk about this and decide that we didn't want to enforce this? I don't know how much time each additional use of useWIndowSize takes but I feel like it is the downstream use of the values as in this PR that may have a bigger impact on performance.

[jimbo]

Didn't we talk about this and decide that we didn't want to enforce this?

I just looked through #1193 and didn't see any discussion of it. The context provider publishes an updated value on each resize event, which causes every consumer to determine whether to re-render. To optimize this, we'd want to debounce in the provider so we were doing less of that determining.

(In other words, subscribers don't control the "stream" of updates. Context updates are distributed via push, not pull, and consumers can't conditionally call useContext().)

The only reason not to debounce in the provider is to protect the edge case in which a hypothetical consumer legitimately needs maximum granularity—literally every resize event. I don't anticipate seeing that edge case in our application.

[sirugh]

@jimbo @supernova-at using height: 100% seems to "fix" the problem but it does add a weird UX when scrolling, which our current scroll lock solution does not account for.

It also breaks the paymentsForm in iOS, which for some reason is nested within the cart footer:

[supernova-at]

It also breaks the paymentsForm in iOS, which for some reason is nested within the cart footer

Yes, this is where height: 100% breaks. I shied away from refactoring the payments form placement out of scope concerns but if we think that'd be an easier fix than all this, I can try that instead?

Edit: Nevermind, I'll take a look at #1449 instead.

[dpatil-magento]

Addressed as part of #1449