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 alignment buttons for embed #840

Merged
merged 1 commit into from
May 19, 2017
Merged

Add alignment buttons for embed #840

merged 1 commit into from
May 19, 2017

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented May 19, 2017

Shamelessly copied from the image block. See #729.

@ellatrix ellatrix requested a review from jasmussen May 19, 2017 10:26
@jasmussen
Copy link
Contributor

Love it! Works great! 👍 👍

@ellatrix
Copy link
Member Author

I notice that the toolbar position is a bit strange when it's "wide", but same for image. Let's address in a separate issue.

@ellatrix ellatrix requested a review from youknowriad May 19, 2017 11:04
@jasmussen
Copy link
Contributor

I notice that the toolbar position is a bit strange when it's "wide", but same for image. Let's address in a separate issue.

This is being addressed largely in #812

@mtias mtias added the [Feature] Blocks Overall functionality of blocks label May 19, 2017
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.

Worth nothing that if we do #830 These controls could be factorized.

LGTM 👍

@ellatrix ellatrix merged commit 3bab12f into master May 19, 2017
@ellatrix ellatrix deleted the add/embed-alignment branch May 19, 2017 12:05
@@ -50,7 +50,7 @@ registerBlock( 'core/image', {
{
icon: 'align-center',
title: wp.i18n.__( 'Align center' ),
isActive: ( { align } ) => 'center' === align,
isActive: ( { align } ) => ! align || 'center' === align,
Copy link
Member

Choose a reason for hiding this comment

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

No align isn't same as center aligned, is 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.

Maybe I'm confusing this... It has the same style.

Copy link
Member

Choose a reason for hiding this comment

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

It appears the same in the editor, but different on the front-end.

none center
none center

We should probably make this distinction more obvious in the editor though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably would be good to get #812 in, before fixing centering issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with reverting that part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants