[feature]: Adding a window size hook #1193

sirugh · 2019-05-02T15:25:37Z

Description

I added a window size hook so that components that need to do something based on window size (recalc/rerender/etc) can do so without having to reimplement the same functionality over and over.

I refactored the Thumbnail component to use the hook in determining whether or not to render the image.

Related Issue

Closes #1192 .

Verification Steps

Open the app using chrome dev tools and set your display to "iPad" in portrait mode.
Navigate to a PDP and see that no thumbnails are requested.
Turn the ipad to landscape mode
See that thumbnails are requested (240px image).

How Have YOU Tested this?

Above verification steps.

Screenshots / Screen Captures (if appropriate):

Proposed Labels for Change Type/Package

major (e.g x.0.0 - a breaking change) * ONLY MAJOR BECAUSE useDocumentListener -> useEventListener
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 · 2019-05-02T15:25:43Z

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-windowsizehook.magento-research1.now.sh

packages/peregrine/src/hooks/__tests__/useWindowSize.spec.js

coveralls · 2019-05-02T15:31:10Z

Coverage decreased (-0.2%) to 76.907% when pulling a2f6c9c on window_size_hook into 1f70740 on develop.

packages/peregrine/src/hooks/useWindowSize.js

sirugh · 2019-05-03T18:10:55Z

packages/venia-concept/src/classify.js

@@ -2,7 +2,7 @@ import React, { Component } from 'react';

 import getDisplayName from 'src/util/getDisplayName';

-const merge = (...args) => Object.assign({}, ...args);
+export const mergeClasses = (...args) => Object.assign({}, ...args);


Another thing taken from #1078

packages/venia-concept/src/components/App/appWrapper.js

packages/venia-concept/src/components/ProductImageCarousel/thumbnail.js

jimbo

Nice work, this is looking good now. Few minor changes to make and we can merge this.

Consider renaming the component to just WindowSize.
Export the component and useWindowSize from index.js in peregrine.
Update the import statements accordingly.

…and use provider at topmost level

sirugh · 2019-05-06T19:19:39Z

Consider renaming the component to just WindowSize.

Should it remain in the hooks folder?

sirugh · 2019-05-06T19:34:45Z

Oh and it seems like this will break a lot of tests unless we add a fake WindowSize context to the test runner root. I'll look into that.

dpatil-magento · 2019-05-10T16:11:16Z

QA Pass.

supernova-at

Removal of the test.only then 👍

packages/peregrine/src/hooks/__tests__/useWindowSize.spec.js

supernova-at · 2019-05-10T16:49:13Z

packages/peregrine/src/hooks/__tests__/useWindowSize.spec.js

+    createTestInstance(
+        <WindowSizeContextProvider>
+            <Component />
+        </WindowSizeContextProvider>


Excuse my lack of education on hooks but I thought part of the idea was that you didn't have to rely on context like this.

Why doesn't it work for Component to just useWindowSize directly?

Okay nevermind, I just reviewed the actual useWindowSize.js file. The reason is because we don't want to add duplicate event listeners, right?

If so, 👍 but it does seem like this pattern could lead to many top-level context wrappers in the future. I'm cool solving that problem if/when we get there though.

If so, 👍 but it does seem like this pattern could lead to many top-level context wrappers in the future. I'm cool solving that problem if/when we get there though.

Yes, it will ideally lead to that. It's like Redux without the slicing.

packages/venia-concept/src/components/ProductImageCarousel/__tests__/carousel.spec.js

sirugh · 2019-05-24T14:12:10Z

This change renames the peregrine useDocumentListener which is a public API. Therefor major is required.

sirugh assigned jimbo May 2, 2019

sirugh commented May 2, 2019

View reviewed changes

packages/peregrine/src/hooks/__tests__/useWindowSize.spec.js Outdated Show resolved Hide resolved

sirugh requested a review from jimbo May 2, 2019 20:55

sirugh unassigned jimbo May 2, 2019

sirugh commented May 2, 2019

View reviewed changes

packages/peregrine/src/hooks/useWindowSize.js Outdated Show resolved Hide resolved

tjwiebell reviewed May 2, 2019

View reviewed changes

packages/peregrine/src/hooks/useWindowSize.js Outdated Show resolved Hide resolved

sirugh force-pushed the window_size_hook branch from a2f6c9c to c9eed84 Compare May 3, 2019 18:10

vercel bot had a problem deploying to staging May 3, 2019 18:10 Failure

sirugh commented May 3, 2019

View reviewed changes

packages/venia-concept/src/components/App/appWrapper.js Outdated Show resolved Hide resolved

vercel bot had a problem deploying to staging May 3, 2019 19:30 Failure

sirugh commented May 3, 2019

View reviewed changes

packages/venia-concept/src/components/ProductImageCarousel/thumbnail.js Outdated Show resolved Hide resolved

supernova-at assigned jimbo May 3, 2019

vercel bot deployed to staging May 6, 2019 14:44 View deployment

jimbo suggested changes May 6, 2019

View reviewed changes

sirugh added 5 commits May 6, 2019 13:49

Add a hook to use window size

c379b77

prettier

b89fca5

Add context and functional app wrapper to use window size hook

ec83fa5

Create and use WindowSizeContext and Provider

61ed490

Combine context, provider and hook in single file. Remove appWrapper …

29d5e2c

…and use provider at topmost level

sirugh force-pushed the window_size_hook branch from 70f69ff to 29d5e2c Compare May 6, 2019 18:53

vercel bot deployed to staging May 6, 2019 18:53 View deployment

Feedback

a4b5e82

vercel bot deployed to staging May 6, 2019 19:28 View deployment

Update tests

7e7fb47

vercel bot deployed to staging May 6, 2019 21:21 View deployment

jimbo previously approved these changes May 10, 2019

View reviewed changes

Merge branch 'develop' into window_size_hook

587c4a2

vercel bot deployed to staging May 10, 2019 13:51 View deployment

Add a comment and be more explicit about the test assertion

e960eff

sirugh dismissed jimbo’s stale review via e960eff May 10, 2019 14:26

vercel bot deployed to staging May 10, 2019 14:26 View deployment

Generalize useEventListener hook

aa9ac9b

vercel bot deployed to staging May 10, 2019 14:55 View deployment

cleanup tests

95bd138

vercel bot deployed to staging May 10, 2019 15:02 View deployment

rename test file

52c4f4a

vercel bot deployed to staging May 10, 2019 15:16 View deployment

supernova-at requested changes May 10, 2019

View reviewed changes

jimbo previously approved these changes May 10, 2019

View reviewed changes

jimbo reviewed May 10, 2019

View reviewed changes

packages/venia-concept/src/components/ProductImageCarousel/__tests__/carousel.spec.js Outdated Show resolved Hide resolved

Update test

acfc914

jimbo dismissed their stale review via acfc914 May 10, 2019 17:23

vercel bot deployed to staging May 10, 2019 17:23 View deployment

jimbo previously approved these changes May 10, 2019

View reviewed changes

Update to use createTestInstance

4377db3

sirugh dismissed jimbo’s stale review via 4377db3 May 10, 2019 17:32

vercel bot deployed to staging May 10, 2019 17:32 View deployment

jimbo approved these changes May 10, 2019

View reviewed changes

supernova-at approved these changes May 10, 2019

View reviewed changes

dpatil-magento merged commit 29cba0c into develop May 10, 2019

sirugh added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label May 24, 2019

sirugh deleted the window_size_hook branch May 24, 2019 14:12

jimbo mentioned this pull request Jul 15, 2019

MiniCart Height Fix #1387

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature]: Adding a window size hook #1193

[feature]: Adding a window size hook #1193

sirugh commented May 2, 2019 •

edited

Loading

vercel bot commented May 2, 2019 •

edited

Loading

coveralls commented May 2, 2019

sirugh May 3, 2019

jimbo left a comment •

edited by sirugh

Loading

sirugh commented May 6, 2019

sirugh commented May 6, 2019

dpatil-magento commented May 10, 2019

supernova-at left a comment •

edited

Loading

supernova-at May 10, 2019

supernova-at May 10, 2019

jimbo May 10, 2019

sirugh commented May 24, 2019

[feature]: Adding a window size hook #1193

[feature]: Adding a window size hook #1193

Conversation

sirugh commented May 2, 2019 • edited Loading

Description

Related Issue

Verification Steps

How Have YOU Tested this?

Screenshots / Screen Captures (if appropriate):

Proposed Labels for Change Type/Package

Checklist:

vercel bot commented May 2, 2019 • edited Loading

coveralls commented May 2, 2019

sirugh May 3, 2019

Choose a reason for hiding this comment

jimbo left a comment • edited by sirugh Loading

Choose a reason for hiding this comment

sirugh commented May 6, 2019

sirugh commented May 6, 2019

dpatil-magento commented May 10, 2019

supernova-at left a comment • edited Loading

Choose a reason for hiding this comment

supernova-at May 10, 2019

Choose a reason for hiding this comment

supernova-at May 10, 2019

Choose a reason for hiding this comment

jimbo May 10, 2019

Choose a reason for hiding this comment

sirugh commented May 24, 2019

sirugh commented May 2, 2019 •

edited

Loading

vercel bot commented May 2, 2019 •

edited

Loading

jimbo left a comment •

edited by sirugh

Loading

supernova-at left a comment •

edited

Loading