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 "Text & Image" Block #9416

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Aug 28, 2018

Description

Context: #6993

This PR implements a block that contains two areas one for a media element (image/video) and one area for blocks (buttons, paragraphs etc). The elements of both areas are vertically aligned.
This PR contains an alternative to #7414. So we can compare the two approaches side by side.
Here instead of middle blocks, the parent block will self-contain the media area (that allows images or videos), so we have only one simple nested area for the content.

Screenshots

sep-04-2018 18-49-22

image

Reviewer notes:

Files under packages/editor/src/utils/media-upload and packages/editor/src/hooks/align.js should not be reviewed in this PR as they are just cherry picks from other PRs: #9707, #9704, and #9634.

Depends on: #9707, #9704, and #9634

@jorgefilipecosta jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Aug 28, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Aug 28, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the add/layout-half-image-containing-centered-contente-media-part-of-parent branch 6 times, most recently from 8e556ed to 83f545d Compare August 31, 2018 19:09
@jorgefilipecosta jorgefilipecosta changed the title [Try] [WIP] Half image Layout - Media Area part of the parent no middle blocks [Try] Half image Layout - Media Area part of the parent no middle blocks Aug 31, 2018
@jorgefilipecosta jorgefilipecosta removed the [Status] In Progress Tracking issues with work in progress label Aug 31, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the add/layout-half-image-containing-centered-contente-media-part-of-parent branch from 83f545d to 5b0e620 Compare September 4, 2018 17:53
},

supports: {
align: [ 'center', 'wide', 'full' ],
Copy link
Member

Choose a reason for hiding this comment

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

Why is center alignment supported? What would that even do for this block?

Copy link
Member Author

Choose a reason for hiding this comment

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

By default, we have a wide alignment for this block the block goes out of the content area if the user does not want that, the center alignment (the default on most other blocks) can be used.

Copy link
Member

@ZebulanStanphill ZebulanStanphill Sep 4, 2018

Choose a reason for hiding this comment

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

@jorgefilipecosta Center is not the default alignment for most blocks. No alignment is the default alignment for most blocks. There is a difference. The Columns block supports wide and full alignments but not the center alignment because the center alignment does not do anything for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The approach I was trying to follow of using center align was a way to unset the alignment is confusing I followed your suggestion and used just wide and full alignments for things to work correctly I needed to apply some engine fixes: #9634

const MEDIA_POSITIONS = [ 'left', 'right' ];
const ALLOWED_BLOCKS = [ 'core/button', 'core/paragraph', 'core/heading', 'core/list' ];
const TEMPLATE = [
[ 'core/paragraph', { fontSize: 'large', placeholder: 'Content...' } ],
Copy link
Member

Choose a reason for hiding this comment

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

This should use an ellipsis (…) rather than 3 periods.

</InspectorControls>
<BlockControls>
<BlockAlignmentToolbar
controls={ MEDIA_POSITIONS }
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to give the media position options different labels from the standard block alignment toolbar? They should probably say "Show media on left" and "Show media on right".

@ianbelanger79 ianbelanger79 added [Type] Enhancement A suggestion for improvement. New Block Suggestion for a new block [Feature] Media Anything that impacts the experience of managing media labels Sep 4, 2018
@jasmussen
Copy link
Contributor

Love the most recent changes. This is going to be a great block. Love:

  • That the block is simple — child blocks that aren't manipulable aren't surfaced.
  • Easy to add images, easy to resize, easy to add text, easy to configure the block

It's delicious!

The drag handle to resize needs a little love. Can we make sure the drag handle is only visible when the block is selected first?

In addition to this, can we change it to use the same resizing handle that images do? I.e. instead of this:

screen shot 2018-09-05 at 09 05 41

We do this:

this

And instead of this:

screen shot 2018-09-05 at 09 08 33

We do this:

this2

I'd also personally be okay if the placeholder spot wasn't resizable, and it wasn't until a media item was added that the resize handle appeared. But it's fine either way.

@jorgefilipecosta jorgefilipecosta force-pushed the add/layout-half-image-containing-centered-contente-media-part-of-parent branch 3 times, most recently from 8b273d2 to b6e8d13 Compare September 5, 2018 19:16
@jorgefilipecosta
Copy link
Member Author

Thank you for your reviews @ZebulanStanphill and @jasmussen all the concerns were addressed :)

@jasmussen
Copy link
Contributor

This is really nice, I'm digging this block a lot:

screen shot 2018-09-06 at 10 46 28

However, possibly a rebase issue, but I'm seeing the title spot way smaller font size than in master:

screen shot 2018-09-06 at 10 45 51

Actually that's also in master.. I will investigate that separately. But 👍 👍 on the design! All this needs is a code review update and good to go I'd say.

@jorgefilipecosta jorgefilipecosta force-pushed the add/layout-half-image-containing-centered-contente-media-part-of-parent branch from b6e8d13 to 80070df Compare September 7, 2018 20:10
@jorgefilipecosta jorgefilipecosta force-pushed the add/layout-half-image-containing-centered-contente-media-part-of-parent branch from 80070df to be42a10 Compare September 10, 2018 10:32
@jorgefilipecosta jorgefilipecosta requested a review from a team September 10, 2018 10:36
@jorgefilipecosta jorgefilipecosta force-pushed the add/layout-half-image-containing-centered-contente-media-part-of-parent branch from 0aa257b to 4a26026 Compare September 10, 2018 14:15
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.

Some small remarks, but overall, looking good to me.

I'd suggest doing a quick test in IE11 and ship.

@jorgefilipecosta jorgefilipecosta force-pushed the add/layout-half-image-containing-centered-contente-media-part-of-parent branch from d71e64b to 33db45f Compare October 19, 2018 11:48
@jorgefilipecosta jorgefilipecosta force-pushed the add/layout-half-image-containing-centered-contente-media-part-of-parent branch from 33db45f to e58c3c8 Compare October 19, 2018 12:09
@jorgefilipecosta jorgefilipecosta merged commit e50efbf into master Oct 19, 2018
@jorgefilipecosta jorgefilipecosta deleted the add/layout-half-image-containing-centered-contente-media-part-of-parent branch October 19, 2018 12:26
@paaljoachim
Copy link
Contributor

paaljoachim commented Oct 22, 2018

I am trying to recreate these sections using the block: http://edwien.no/publikasjoner/

Result in the backend:
screen shot 2018-10-22 at 17 54 17

Image has space above and below. Heading aligns somewhat to the top of the image. Text links aligns somewhat to the bottom of the image.

In the frontend it looks like this (clicking the preview button):
screen shot 2018-10-22 at 17 55 06

The frontend will show the image nudged up top, left and it is going below the edge of the box.
Heading and text shows up in the center of the image.

Another:
Backend:
screen shot 2018-10-22 at 18 03 03

Frontend:
screen shot 2018-10-22 at 18 03 43

Here it might be good to be able to set the height of the block through the block setting.

@lkraav
Copy link

lkraav commented Oct 22, 2018

Testing it today, I wasn't sure why I wasn't able to add a Quote block in the Text container.

jorgefilipecosta added a commit that referenced this pull request Oct 23, 2018
…0913)

## Description
Fixes: #10895
On unified toolbar mode, it was not possible to resize the media on Media & Text block.
During the elaboration of #9416 this bug was addressed but I think during subsequent rebases and changes more concretely the usage of our abstracted Resizable box we had a regression.

This PR applies exactly the same fix that was applied in spacer block to the Media & Text block.


## How has this been tested?
I checked it is possible to resize the media in Media & Text block in all modes including the unified toolbar.
youknowriad pushed a commit that referenced this pull request Oct 24, 2018
…0913)

## Description
Fixes: #10895
On unified toolbar mode, it was not possible to resize the media on Media & Text block.
During the elaboration of #9416 this bug was addressed but I think during subsequent rebases and changes more concretely the usage of our abstracted Resizable box we had a regression.

This PR applies exactly the same fix that was applied in spacer block to the Media & Text block.


## How has this been tested?
I checked it is possible to resize the media in Media & Text block in all modes including the unified toolbar.
@webmandesign
Copy link
Contributor

webmandesign commented Oct 25, 2018

Hi guys,

I certainly don't want to sound disrespectful, that's not my intention here. I just have feeling this block might be kind of redundant.

You see, if Columns block allowed setting up a all/each column background and text color, and vertical alignment (which is definitely possible as columns are using styled using flexbox) I thing we could replicate Media & Text block's functionality that way and yet allow more flexibility still (as from what I know I can use any block inside column).

However, I understand having a dedicated Media & Text block simplifies the setup of such layout section, as my approach would require some much needed improvements to Columns block setup.

That's just my thoughts…

Additional suggestion: If Media & Text block is here to stay, I suggest adding an option to set up also a text color, not just background color. Thanks!

@paaljoachim
Copy link
Contributor

The Media & Text block seems like a precursor to the section/container block:
#4900

antpb pushed a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018
This commit implements a block that contains two areas one for a media element (image/video) and one area for blocks (buttons, paragraphs etc). The elements of both areas are vertically aligned.
Here instead of middle blocks, the parent block will self-contain the media area (that allows images or videos), so we have only one simple nested area for the content.
antpb pushed a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018
…rdPress#10913)

## Description
Fixes: WordPress#10895
On unified toolbar mode, it was not possible to resize the media on Media & Text block.
During the elaboration of WordPress#9416 this bug was addressed but I think during subsequent rebases and changes more concretely the usage of our abstracted Resizable box we had a regression.

This PR applies exactly the same fix that was applied in spacer block to the Media & Text block.


## How has this been tested?
I checked it is possible to resize the media in Media & Text block in all modes including the unified toolbar.
jorgefilipecosta added a commit that referenced this pull request Nov 7, 2018
…s. (#10696)


Currently, if a block contains a template when we insert that block the block that gets focused is one of the child blocks specified in the template. We don't offer a way to disable this behavior.

This current behavior makes sense in most cases (e.g: columns) but for some cases,
we may want to keep the focus on the parent. E.g. In PR #9416 @afercia pointed some a11y concerns in automatically selecting the child block on the InnerBlocks area.

Blocks may have their own edition area and an InnerBlocks are at the end. Automatically selecting a block in the InnerBlocks area, has some accessible concerns has the parent block edition area may be unnoticeable for screen reader users.

This PR adds a flag to the InnerBlocks component that allows blocks to disable the behavior of automatically selecting the children. In order for this flag to be possible another flag was added in insertBlock(s) action that allows for blocks to be inserted without a selection update happening.
jorgefilipecosta added a commit that referenced this pull request Nov 14, 2018
Currently, we have a bug even if a block has a paragraph as the last block of the template if no locking exists another paragraph is inserted.

This happens because, before the template is processed the parent block is empty and DefaultBlockApppender is rendered right away. DefaultBlockApppender inserts blocks when it gets focused. The parent block has a mechanism to focus the first table and if InnerBlocks were not yet processed and the parent block does not contain inputs the DefaultBlockApppender is going to get the focus.

Before the template is processed we don't know what blocks will exist so during this phase DefaultBlockApppender should not be rendered. This PR makes sure that we don't render DefaultBlockApppender right after the block is inserted before the template is processed.

This behavior was noticed in #9416.




## How has this been tested?
I added the test block available in https://gist.github.com/jorgefilipecosta/edafb2422ef41020d75619adf31d725e.

I checked that after inserting the first paragraph specified in the template gets focused and no other paragraph is created.

I checked the blocks depending on the behavior of DefaultBlockApppender inserting a block (columns) continue to work as before.


## Screenshots <!-- if applicable -->
test block: https://gist.github.com/jorgefilipecosta/edafb2422ef41020d75619adf31d725e
```
	var TEMPLATE = [
		[ 'core/paragraph', { placeholder: 'Paragraph 1', customFontSize: 35 } ],
	];
```
After:

<img width="568" alt="screen shot 2018-10-18 at 13 07 03" src="https://user-images.githubusercontent.com/11271197/47153548-e36a5b80-d2d7-11e8-83a6-33a9b39d97ce.png">

<img width="700" alt="screen shot 2018-10-18 at 13 06 54" src="https://user-images.githubusercontent.com/11271197/47153560-e9f8d300-d2d7-11e8-8086-cfa2e2252432.png">
Before:

<img width="675" alt="screen shot 2018-10-18 at 13 17 27" src="https://user-images.githubusercontent.com/11271197/47153679-30e6c880-d2d8-11e8-82b4-d4e7c19fa0b9.png">
<img width="656" alt="screen shot 2018-10-18 at 13 17 21" src="https://user-images.githubusercontent.com/11271197/47153683-33e1b900-d2d8-11e8-80da-7afc0294d311.png">
}
return (
<ResizableBox
className="editor-media-container__resizer"
Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media Anything that impacts the experience of managing media [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). New Block Suggestion for a new block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.