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

Use a light block DOM for the Media & Text block #23062

Merged
merged 2 commits into from
Jun 22, 2020

Conversation

oxyc
Copy link
Member

@oxyc oxyc commented Jun 10, 2020

Description

This PR ultimately makes the Media & Text editor markup match that of the frontend by:

How has this been tested?

Tested adding video as well as image and toggling all the attributes to see that everything is working.

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ZebulanStanphill ZebulanStanphill added [Block] Media & Text Affects the Media & Text Block [Type] Code Quality Issues or PRs that relate to code quality [Package] Block library /packages/block-library labels Jun 10, 2020
@oxyc
Copy link
Member Author

oxyc commented Jun 11, 2020

Let me know if i should split this into multiple PRs instead.

@ellatrix
Copy link
Member

Thanks for the PR, @oxyc! Would it be possible to split it? Maybe one PR converting it to hooks and one for the light block wrapper?

@oxyc
Copy link
Member Author

oxyc commented Jun 11, 2020

Thanks for the PR, @oxyc! Would it be possible to split it? Maybe one PR converting it to hooks and one for the light block wrapper?

Sure #23102. I'll update this PR once that one is merged.

@oxyc
Copy link
Member Author

oxyc commented Jun 11, 2020

Rebased it on top of #23102 (will keep it updated as that pr gets fixed). Commits on top:

@oxyc
Copy link
Member Author

oxyc commented Jun 16, 2020

Ready for review. I've tested it with twentynineteen, twentytwenty and my own theme and couldn't find any issues at least.

Note, since I'm new to lerna, please double check that the package-lock.json was updated correctly with lerna boostrap

DOM on editor side after commit:

<div aria-label="Block: Media &amp; Text" role="group" class="wp-block-media-text custom-class block-editor-block-list__block is-selected has-media-on-the-right is-selected is-stacked-on-mobile is-vertically-aligned-bottom is-image-fill has-primary-background-color has-background" id="block-a290aa4c-0230-4530-b501-997effb88396" data-block="a290aa4c-0230-4530-b501-997effb88396" data-type="core/media-text" data-title="Media &amp; Text" tabindex="0" style="grid-template-columns: 1fr 51%; transform-origin: center center;">
  <figure class="components-resizable-box__container has-show-handle wp-block-media-text__media editor-media-container__resizer" axis="x" style="position: relative; user-select: auto; width: 51%; height: auto; max-width: 100%; min-width: 10%; box-sizing: border-box; flex-shrink: 0; background-image: url(&quot;http://shs.test/app/uploads/2020/06/copy-of-hankenvappen-200x2x-1024x446.png&quot;); background-position: 78% 73%;">
    <img src="/app/uploads/2020/06/copy-of-hankenvappen-200x2x-1024x446.png" alt="test">
    <span>
      <div class="components-resizable-box__handle components-resizable-box__side-handle components-resizable-box__handle-left" style="position: absolute; user-select: none; cursor: col-resize;"></div>
    </span>
  </figure>
  <div class="block-editor-block-list__layout wp-block-media-text__content">
    <h3 aria-label="Write heading…" role="textbox" class="block-editor-block-list__block wp-block rich-text block-editor-rich-text__editable wp-block" aria-multiline="" contenteditable="true" id="block-f99f0733-fd83-4ef4-a63f-52d73fda8f58" data-block="f99f0733-fd83-4ef4-a63f-52d73fda8f58" data-type="core/heading" data-title="Heading" tabindex="0" style="white-space: pre-wrap; transform-origin: center center;"></h3>
    <div tabindex="-1" class="block-list-appender"></div>
  </div>
  <div class="__resizable_base__" style="width: 100%; height: 100%; position: absolute; transform: scale(0, 0); left: 0px; flex: 0 1 0%;"></div>
</div>

@oxyc
Copy link
Member Author

oxyc commented Jun 17, 2020

Seems like some bugs are fixed in the process as well.

Expected (frontend):

Screen Shot 2020-06-17 at 13 56 05

Before (editor):

Screen Shot 2020-06-17 at 13 58 42

After (editor):

Screen Shot 2020-06-17 at 13 47 13

@ZebulanStanphill ZebulanStanphill added the [Type] Bug An existing feature does not function as intended label Jun 17, 2020
@oxyc
Copy link
Member Author

oxyc commented Jun 17, 2020

Note if testing twentynineteen. The custom colors aren't shown because of #21931

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This is a cool PR. Thanks @oxyc
Code changes look good, I'd appreciate another review to ensure there's no regressions. cc @jasmussen @ZebulanStanphill but otherwise, this is cool.

Copy link
Member

@ZebulanStanphill ZebulanStanphill left a comment

Choose a reason for hiding this comment

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

I tried out the branch and everything seems to be working great!

@ZebulanStanphill
Copy link
Member

(Try rebasing the branch to get tests to pass. There have been some issues with end-to-end tests lately.)

@noisysocks noisysocks merged commit 4825ad1 into WordPress:master Jun 22, 2020
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 22, 2020
This was referenced Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Media & Text Affects the Media & Text Block [Package] Block library /packages/block-library [Type] Bug An existing feature does not function as intended [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants