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

Add ability to show inline media controls for the Image component #172

Closed
wants to merge 12 commits into from

Conversation

ncoetzer
Copy link
Contributor

Description of the Change

Sometimes, we have blocks that require multiple images, and this change aims at offering an inline solution for replacing and or removing images where the toolbar replaces flow isn't ideal.

Closes #164

How to test the Change

Steps to test

  • Add the Multiple Image Example block to the page.

image

  • You should see media placeholders for 3 images.

image

  • Once you have set an image, you should see the "Replace" and "Remove" icon buttons when hovering over the image. This is because the new (optional) hasInlineControls props have been set to true.

image

Note: If text is preferred over icons, this could be easily replaced. We should probably get UX/Design feedback on the icons.

  • You should be able to replace the image with another and see the updated image display (updating the image id in the DB)
  • The "Remove" icon button should not be visible if the isOptional props have been set to false.

image

Usage example:

Optional
<Image 
  id={imageId} 
  size="large" 
  onSelect={(image) => setAttributes({imageId: image.id })} 
  className="example-image" 
  focalPoint={focalPoint} 
  onChangeFocalPoint={(value) => setAttributes({focalPoint: value})} 
  onRemove={() => setAttributes({imageId: null})} 
  hasInlineControls={true} 
/>
Required
<Image 
  id={imageId} 
  size="large" 
  onSelect={(image) => setAttributes({imageId: image.id })} 
  className="example-image" 
  focalPoint={focalPoint} 
  onChangeFocalPoint={(value) => setAttributes({focalPoint: value})} 
  onRemove={() => setAttributes({imageId: null})}
  hasInlineControls={true} 
  isOptional={false}
/>

Changelog Entry

Added - Ability to show inline media controls for the Image component

Credits

Props @fabiankaegy

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@ncoetzer ncoetzer self-assigned this Nov 11, 2022
@ncoetzer ncoetzer changed the title Feature/image inline controls option Add ability to show inline media controls for the Image component Nov 11, 2022
@ncoetzer ncoetzer changed the title Add ability to show inline media controls for the Image component Add ability to show inline media controls for the Image component Nov 11, 2022
Copy link
Member

@fabiankaegy fabiankaegy left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @ncoetzer. This will be such a massive improvement for the image component.

I had asked @xavortm to mock up some designs and think through some edge cases and he provided a few mockups here: #164 (comment)

I would love your thoughts on them so we can update this PR to match that design.


I also have one more significant concern that I am not sure how we can best address it. Something that we should always try with these components is markup parity with the frontend of the site. Which in this case means outputting an img tag. This way all the custom styles that get applied from the theme/block author actually get picked up by the image same as they do on the frontend. without creating additional work to stye the admin.

The way this currently works we lose this parity because we need to wrap the img tag in a div in order to absolutely position the button elements on top of the image.

I'm not sure what a good solution for this is yet. But I would like us to experiment with either moving the button elements to become siblings of the image tag and then using JS in the editor to handle the positioning similar to how Gutenberg does it with popovers etc. (Maybe we can even use the popover with an anchorRef and style it so it looks like it is inline.)

Let me know if you want to brainstorm on this together :) Also don't feel the need to sync time into this. I'm also happy to pick this up from here if you don't have any more time to spend on this :)

example/src/blocks/multiple-image-example/edit.js Outdated Show resolved Hide resolved
@fabiankaegy fabiankaegy marked this pull request as draft December 15, 2022 10:42
@ncoetzer ncoetzer marked this pull request as ready for review July 17, 2023 10:38
@github-actions
Copy link

Size Change: +1.94 kB (+4%)

Total Size: 54.8 kB

Filename Size Change
dist/index.js 54.8 kB +1.94 kB (+4%)

compressed-size-action

@fabiankaegy
Copy link
Member

Hey @ncoetzer 👋

Screen.Recording.2023-07-17.at.12.43.59.PM.mp4

The new look you shared for this component is sooooo nice! I really love it! :) Great work!

The only thing I still struggle with, is that this change introduces a new additional div in the markup, which I would consider a breaking change since it might mess with existing style setups that expect this component to just render the img tag.

Because of that I think we have two available options:

  1. Refactor the code here to get rid of the additional component. You can look how @darrenjacoby achieved this in New component: FieldValidation #238 (useFloating + cloneElement) so that the output rendered by the Image component still is only the img tag
  2. Abandon this PR here and instead move this new code into the exploration started in Draft: Refactor Image component to a more composable API #222

@fabiankaegy
Copy link
Member

Closing in favor of #269

@fabiankaegy fabiankaegy deleted the feature/image-inline-controls-option branch February 11, 2024 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a new hasInlineControls option to Image component
2 participants