Enhancement to issue #28859

[Quintis1212]

Hello everyone ) this is enhancement for #28859 issue. I made a focus settings specific to an image and reset to a neutral position when a new image is uploaded

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @Quintis1212! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[stokesman]

Hi @Quintis1212, thanks for your work on this.

I was surprised by the approach here as I would have thought to handle this within the Cover block and not in the focal point picker component. But then it seems you've intended to make an extra enhancement beyond what (I interpret) is needed to resolve the issue. I can see how the title of the issue might suggest it though.

As I interpret it, the needed solution is to reset the focal point when changing the media. This PR does that. The extra enhancement would be to restore a previously set focal point for a previously used image. Though, that part didn't work for me when I tested this.

I think it'd be best to narrow the focus of this PR to just make the reset and for that, I don't think the focal point picker component should be changed. That's because there should be a solution with fewer changes. At present, the single place this enhancement is needed is the Cover block. The only other use of the focal point picker is in the Media & Text block and that block already resets the focal point when the media changes. The same behavior in the Cover block could be had by adding a single line. Note that the Cover block already resets the focal point when changing the media if the type of media is a video:

gutenberg/packages/block-library/src/cover/shared.js

Lines 100 to 107 in 26560b6

	setAttributes( {
	url: media.url,
	id: media.id,
	backgroundType: mediaType,
	...( mediaType === VIDEO_BACKGROUND_TYPE
	? { focalPoint: undefined, hasParallax: undefined }
	: {} ),
	} );

That's my take and hopefully, it makes sense to you as well.

[Quintis1212]

Hi @Quintis1212, thanks for your work on this.

I was surprised by the approach here as I would have thought to handle this within the Cover block and not in the focal point picker component. But then it seems you've intended to make an extra enhancement beyond what (I interpret) is needed to resolve the issue. I can see how the title of the issue might suggest it though.

As I interpret it, the needed solution is to reset the focal point when changing the media. This PR does that. The extra enhancement would be to restore a previously set focal point for a previously used image. Though, that part didn't work for me when I tested this.

I think it'd be best to narrow the focus of this PR to just make the reset and for that, I don't think the focal point picker component should be changed. That's because there should be a solution with fewer changes. At present, the single place this enhancement is needed is the Cover block. The only other use of the focal point picker is in the Media & Text block and that block already resets the focal point when the media changes. The same behavior in the Cover block could be had by adding a single line. Note that the Cover block already resets the focal point when changing the media if the type of media is a video:

gutenberg/packages/block-library/src/cover/shared.js

Lines 100 to 107 in 26560b6

	setAttributes( {
	url: media.url,
	id: media.id,
	backgroundType: mediaType,
	...( mediaType === VIDEO_BACKGROUND_TYPE
	? { focalPoint: undefined, hasParallax: undefined }
	: {} ),
	} );

That's my take and hopefully, it makes sense to you as well.

Hi @stokesman , i add new code to shared.js in cover block as you advised:
`
setAttributes( {
url: media.url,
id: media.id,
backgroundType: mediaType,
...( mediaType === VIDEO_BACKGROUND_TYPE
? { focalPoint: undefined, hasParallax: undefined }
: {} ),
...( mediaType === IMAGE_BACKGROUND_TYPE
? { focalPoint: undefined, hasParallax: undefined }
: {} ),
} );

`

But its work with errors :

[stokesman]

@Quintis1212, That error is odd. I just tested that change on both master and then even this branch and couldn't reproduce the console error 😕 . Besides that, this would be the more appropriate change:

		setAttributes( {
			url: media.url,
			id: media.id,
			backgroundType: mediaType,
			focalPoint: undefined,
			...( mediaType === VIDEO_BACKGROUND_TYPE
				? { hasParallax: undefined }
				: {} ),
		} );

It makes the reset of focalPoint unconditional (since both media types should reset that).

Maybe restarting the npm run dev could clear up the issue. That or there's something else going on in your branch and perhaps a rebase on master could help.

[annezazu]

@Quintis1212 is work continuing here on your PR? For now, going to mark as stale :) Let me know if there's any support that can be provided to help you merge any of your open PRs.

[Quintis1212]

@annezazu Hi . I am closing this PR