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

Draft: Refactor Image component to a more composable API #222

Draft
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

fabiankaegy
Copy link
Member

@fabiankaegy fabiankaegy commented May 31, 2023

Description of the Change

This is still an early draft and needs more work

Refactor the Image component to include the figure element in the markup of the Image component in the editor. This reason for this change is to make it easier to add additional elements inside the image component that are absolutely positioned in relation to the wrapping figure. Examples of this include the Image Drop Zone / media replacement controls as explored in #172 and #218

<Image id="2" className="example" onChange={...} />

Output Before:

<img src="..." alt="..." class="example">

Output After:

<figure>
    <img src="..." alt="..." class="example">
    <div class="component-drop-zone">...</div>
</figure>

This however introduces new issues:

  1. There might be instances where we need to include additional elements such as a figcaption inside the figure
  2. We will need to also support adding additional props to the wrapping figure
  3. There might be instances where you need to only render the ´img` tag without anything else.

In order to solve this the Image component now also comes with a few child components that can be used to take control over the full markup.

<Image id="2" onChange={...}>
    <Image.Figure className="example-wrapper">
        <Image.Image className="example-image" />
        <figcaption>Example</figcaption>
    </Image.Figure>
</Image>

Output:

<figure class="example-wrapper">
    <img src="..." alt="..." class="example-image">
    <figcaption>Example</figcaption>
    <div class="component-drop-zone">...</div>
</figure>

Or alternatively, you can also use a render function to completely take over the rendering:

<Image id="2" onChange={...}>
    { ( imageUrl ) => (
        <div style={{backgroundImage: 'url(imageUrl)'}} />
    ) }
</Image>

Closes #

How to test the Change

Changelog Entry

Added - New feature
Changed - Existing functionality
Deprecated - Soon-to-be removed feature
Removed - Feature
Fixed - Bug fix
Security - Vulnerability

Credits

Props @fabiankaegy @Sidsector9 @ncoetzer

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.

@github-actions
Copy link

github-actions bot commented May 31, 2023

Size Change: +961 B (+1%)

Total Size: 66.1 kB

Filename Size Change
dist/index.js 66.1 kB +961 B (+1%)

compressed-size-action

@Antonio-Laguna
Copy link
Member

Just a tiny suggestion that you're completely welcome to ignore. Instead of Image.Image thoughts on Image.Media?

@fabiankaegy
Copy link
Member Author

@Antonio-Laguna yeah I hate Image.Image =D

The reason I didn't go with Image.Media initially is because @benlk is currently looking into creating a Video component and so I wanted to ensure we don't create confusion with the term media...

But maybe we should approach this differently 🤔 Instead of creating a breaking change with this image component here what if we introduce a new Media component which can support Media.Image and Media.Video subcomponents 🤔

Not sure whether it would actually work but may be an option 🤔

Or just Image.Img to match the actual tag name 🤔

@benlk
Copy link
Contributor

benlk commented Jun 1, 2023

If we have an Image for <img>, why not call the new <figure> component Figure?

@cypress
Copy link

cypress bot commented Aug 23, 2023

5 failed tests on run #705 ↗︎

5 1 0 0 Flakiness 0

Details:

Merge pull request #269 from 10up/feature/include-figure-in-image-component-inli...
Project: 10up Block Components Commit: e845203ba8
Status: Failed Duration: 01:24 💡
Started: Jan 29, 2024 8:21 AM Ended: Jan 29, 2024 8:23 AM
Failed  IconPicker.spec.js • 1 failed test

View Output Video

Test Artifacts
IconPicker > allows the user to use the post picker to change an icon and displays it Screenshots Video
Failed  Image.spec.js • 1 failed test

View Output Video

Test Artifacts
Image > allows the user to pick an image from the media library and displays it inline Screenshots Video
Failed  Link.spec.js • 1 failed test

View Output Video

Test Artifacts
Link > allows the editor to pick a link directly inline Screenshots Video
Failed  Repeater.spec.js • 1 failed test

View Output Video

Test Artifacts
Repeater > Adding Repeater field and saving it. Screenshots Video
Failed  registerBlockExtension.spec.js • 1 failed test

View Output Video

Test Artifacts
registerBlockExtension > ensure the new setting shows up and doesn't cause deprecation errors Screenshots Video

Review all test suite changes for PR #222 ↗︎

@fabiankaegy fabiankaegy self-assigned this Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants