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

Image: Manually update resizable box state when we don't have dimenssions #29919

Merged
merged 2 commits into from
Mar 26, 2021

Conversation

Mamaduka
Copy link
Member

Description

When changing to a different image, we reset dimension attributes, including size props for the resizable box. But this doesn't automatically reset dimensions in the resizable box state, which is used for inline styles. So we have to do it manually.

Fixes #23985.

P.S. This might be an issue with the upstream package - re-resizable.

How has this been tested?

  1. Add image block and set image
  2. Resize the image.
  3. Replace it with a different image.
  4. See if a resizable box is reset.

Types of changes

Bugfix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • [N/A] My code follows the accessibility standards.
  • [N/A] I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • [N/A] I've updated all React Native files affected by any refactorings/renamings in this PR.

@Mamaduka Mamaduka requested a review from ajlende as a code owner March 17, 2021 03:12
@Mamaduka Mamaduka added [Block] Image Affects the Image Block [Feature] Media Anything that impacts the experience of managing media [Type] Bug An existing feature does not function as intended labels Mar 17, 2021
@Mamaduka Mamaduka self-assigned this Mar 18, 2021
Comment on lines 183 to 190
useEffect( () => {
if ( url && resizableBoxRef.current && ! width && ! height ) {
resizableBoxRef.current.updateSize( {
width: 'auto',
height: 'auto',
} );
}
}, [ url, width, height ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this being called in a useEffect instead of in updateImage where the attributes are reset?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or better yet, could we just set the width and height of the ResizableBox component to be 'auto' when attributes.width and attributes.height are undefined?

<ResizableBox
  size={ {
    width: width ?? 'auto',
    height: height ?? 'auto',
  } }
  ...
/>

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, @ajlende

Thanks for the review.

Why is this being called in a useEffect instead of in updateImage where the attributes are reset?

The attributes are reset in onSelectImage and updateImage. Using the useEffect seemed more optimal at the time.

Or better yet, could we just set the width and height of the ResizableBox component to be 'auto' when attributes.width and attributes.height are undefined?

Thanks for suggesting a clearer approach 🙇 This is the reason I love reviews.

…eight

When changing to a different image, we reset dimension attributes, including size props for the resizable box. But this doesn't automatically reset dimensions in the resizable box state, which is used for inline styles. So we have to do it manually.

See WordPress#23985.
@Mamaduka Mamaduka force-pushed the fix/image-reset-resizable-box branch from 4f7be48 to de97b1a Compare March 26, 2021 04:35
Copy link
Contributor

@ajlende ajlende left a comment

Choose a reason for hiding this comment

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

Looks good to me!

29919.mp4

@Mamaduka Mamaduka merged commit 12cd455 into WordPress:trunk Mar 26, 2021
@github-actions github-actions bot added this to the Gutenberg 10.4 milestone Mar 26, 2021
@Mamaduka Mamaduka deleted the fix/image-reset-resizable-box branch March 26, 2021 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Feature] Media Anything that impacts the experience of managing media [Type] Bug An existing feature does not function as intended
Projects
None yet
2 participants