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

Updates the image's container size when we resize the window #6205

Merged
merged 2 commits into from Apr 17, 2018

Conversation

Projects
None yet
3 participants
@youknowriad
Contributor

youknowriad commented Apr 16, 2018

closes #6191

This PR fixes the resizing behavior of image blocks when we resize the browser size.

Testing instructions

  • Repeat the instructions in #6191
  • The image should scale according to its container size.

@youknowriad youknowriad self-assigned this Apr 16, 2018

@youknowriad youknowriad requested a review from jasmussen Apr 16, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 16, 2018

Not sure if I've checked this branch out correctly, but it doesn't seem to be working for me.

resize

While it sounds like setting the image size on resize, I'm not sure that would fix #6191. The ideal behavior is actually that the dimensions on an un-resized image aren't set at all, not that they are re-calculated per the viewport width. The really weird thing is that this works as expected <782px, and the dimensions seem to get added >782px.

Make sense?

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 16, 2018

Ok, so this is happening because of the "fit-content width" of the image wrapper. Before we were checking the wrapper's size, but since the wrapper's size don't change with this width, it's creating issues.

Not an easy fix, still looking. I think there's a reason for us setting the size of the image even if we want it to take the parent's size (related to the resizing behavior) I'll see if removing them fixes the issues.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 16, 2018

Removed the explicit width/height. I can't see regressions but please help testing the resizing behavior :)

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 16, 2018

Nice, this seems to work perfectly with no regressions!

The inline styles are still technically applied to the resizablebox div only when the viewport is >782px, though, and I don't understand why they instantiate there but not earlier. But if we can't figure out why those are applied, the fix you just pushed seems solid enough, and appears to solve the issue!

Do you know, perchance, why those styles get applied at that breakpoint and not before? If they are there for a reason, then this is definitely good to ship. If not, then I'm a little reluctant that there's another regression that this is a bandaid to.

Thanks for your work!

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 16, 2018

@jasmussen I guess because we disable the resizable box entirely for small viewports.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 16, 2018

I guess because we disable the resizable box entirely for small viewports.

Awesome, that sounds right, then we know what's up. I think this is good to ship, experience wise, and it solves a real problem in master right now. Thanks so much for this, and CC: @noisysocks, as it fixes the issue I mentioned in your branch.

@noisysocks

Makes sense and fixes the issue!

@@ -79,4 +80,6 @@ class ImageSize extends Component {
}
}
export default ImageSize;
export default withGlobalEvents( {

This comment has been minimized.

@noisysocks

noisysocks Apr 17, 2018

Member

I think this is my favourite HOC so far ❤️

This comment has been minimized.

@youknowriad
width: currentWidth,
height: currentHeight,
width,
height,
} }

This comment has been minimized.

@noisysocks

noisysocks Apr 17, 2018

Member

It probably doesn't matter in practice but the docs say that size is optional and that .width and .height are required.

<ResizableBox
    size={ width && height && {
        width,
        height,
    } }
    ...
/>

@youknowriad youknowriad merged commit f10da4d into master Apr 17, 2018

2 checks passed

codecov/project 43.94% remains the same compared to fc15186
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/image-container-size-on-window-resize branch Apr 17, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 17, 2018

YOU ARE THE AWESOMEST

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