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

MiniCart Height Fix #1387

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/peregrine/src/hooks/useRestApi.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const useRestApi = endpoint => {
receiveError(error.baseMessage);
}
},
[receiveError, receiveResponse, setLoading]
[endpoint, receiveError, receiveResponse, setLoading]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

);

const api = useMemo(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`renders the correct tree 1`] = `
<aside>
<aside
style={
Object {
"--minicart-height-unit": "7.19px",
}
}
>
<Header
closeDrawer={[MockFunction]}
isEditingItem={false}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import React from 'react';
import ShallowRenderer from 'react-test-renderer/shallow';

import { useWindowSize } from '@magento/peregrine';

import MiniCart from '../miniCart';

const renderer = new ShallowRenderer();
Expand Down Expand Up @@ -43,6 +45,10 @@ const baseProps = {
updateItemInCart: jest.fn()
};

beforeAll(() => {
useWindowSize.mockReturnValue({ innerHeight: 719 });
});

test('renders the correct tree', () => {
const tree = renderer.render(<MiniCart {...baseProps} />);

Expand Down
5 changes: 4 additions & 1 deletion packages/venia-concept/src/components/MiniCart/miniCart.css
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
.root {
--base-z-index: 4;
--minicart-height: 100vh;
supernova-at marked this conversation as resolved.
Show resolved Hide resolved
--minicart-header-height: 3.5rem;
--minicart-height: calc(
var(--minicart-height-unit, 1vh) * 100
); /* Note: --minicart-height-unit set by JS */
align-content: start;
background-color: white;
bottom: 0;
box-shadow: -1px 0 rgb(var(--venia-border));
display: grid;
grid-template-rows: min-content 1fr;
height: 100vh; /* fallback for browsers that don't support custom properties */
height: var(--minicart-height);
opacity: 0;
overflow: hidden;
Expand Down
30 changes: 28 additions & 2 deletions packages/venia-concept/src/components/MiniCart/miniCart.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import React from 'react';
import React, { useEffect, useRef, useState } from 'react';
import { arrayOf, bool, func, number, object, shape, string } from 'prop-types';
import debounce from 'lodash.debounce';

import { useWindowSize } from '@magento/peregrine';

import Body from './body';
import Footer from './footer';
Expand All @@ -26,12 +29,35 @@ const MiniCart = props => {
} = props;
const { editItem, isEditingItem, isLoading, isUpdatingItem } = cart;

// Hooks.
const { innerHeight } = useWindowSize();
const [viewportHeight, setViewportHeight] = useState(innerHeight);
// We need innerHeight to be mutable so its value is not captured in
// our debounced function.
const innerHeightMutableRef = useRef(innerHeight);

// Always update the innerHeight mutable ref with innerHeight's latest value.
innerHeightMutableRef.current = innerHeight;

const setViewportHeightDebounced = debounce(() => {
// We're really just doing setViewportHeight(innerHeight) here
// but because we're within a debounced function, innerHeight
// gets captured (never updates).
// So we use this ref instead to ensure we always have the latest value.
setViewportHeight(innerHeightMutableRef.current);
}, 1000);
const setViewportHeightDebouncedRef = useRef(setViewportHeightDebounced);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not just useMemo here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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...

useEffect(setViewportHeightDebouncedRef.current, [innerHeight]);

// Members.
const classes = mergeClasses(defaultClasses, props.classes);
const currencyCode = getCurrencyCode(cart);
const cartItems = cart.details.items;
const numItems = cart.details.items_qty;
const rootClass = isOpen ? classes.root_open : classes.root;
const rootStyle = {
'--minicart-height-unit': `${viewportHeight * 0.01}px`
};
const subtotal = cart.totals.subtotal;

const showFooter = !(isCartEmpty || isLoading || isEditingItem);
Expand All @@ -46,7 +72,7 @@ const MiniCart = props => {
) : null;

return (
<aside className={rootClass}>
<aside className={rootClass} style={rootStyle}>
<Header closeDrawer={closeDrawer} isEditingItem={isEditingItem} />
<Body
beginEditItem={beginEditItem}
Expand Down