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

Blocks: Extract and factorize block alignment controls #1019

Merged
merged 1 commit into from
Jun 19, 2017

Conversation

youknowriad
Copy link
Contributor

This PR continues our Refactoring to move the controls to the edit function.

Testing instructions

  • Block Alignment controls for the button, image, embed and table block should continue the work the way they work on master.

@youknowriad youknowriad added the [Feature] Blocks Overall functionality of blocks label Jun 5, 2017
@youknowriad youknowriad self-assigned this Jun 5, 2017
@youknowriad youknowriad requested a review from aduth June 5, 2017 13:32
@aduth
Copy link
Member

aduth commented Jun 5, 2017

Is this different in purpose than #987 ? How does it relate?

@youknowriad
Copy link
Contributor Author

Isn't #987 about text alignments? or do you think we can reuse it?

@aduth
Copy link
Member

aduth commented Jun 5, 2017

#987 is mainly toward a goal of letting alignment be a consideration of the block as a whole, not individual paragraphs. So seems similar in purpose to what you've proposed here, though your changes include a handful more blocks.

@youknowriad
Copy link
Contributor Author

But don't you think we should keep distinction between TextAlignmentToolbar and BlockAlignmentToolbar ? Even if the internals are close, and could share some code, having two different components could be a good thing.

@aduth
Copy link
Member

aduth commented Jun 5, 2017

Hmm, what would be the practical difference between the two? Seems they only really vary in the icons they render. Both assume a callback is to be passed to specify the actual change implementation.

@youknowriad
Copy link
Contributor Author

yes, the icons are the only differences but sometimes it's good to duplicate code for clarity. A component could have both. These are the two toolbars we have for now, but we may have other reusable toolbars a block author could use. I see them as a meaningful group of controls instead of a more generic toolbar component with maybe unrelated controls to pick.

Anyway, this is just an idea, the generic "toggle" toolbar is a possibility too.

@aduth
Copy link
Member

aduth commented Jun 5, 2017

Yeah, not so opposed to the idea of duplication as much as questioning whether a distinction between text alignment vs. block alignment is really one we care to make, or is in-fact more clear.

Maybe cc @jasmussen @mtias to this point.

@youknowriad
Copy link
Contributor Author

I'm keen to move these two (PRs #987 and this one) forward to remove the controls property from the block API.

@youknowriad youknowriad force-pushed the update/block-alignment-controls branch from 6dd1048 to 0a6f728 Compare June 8, 2017 10:40
aduth
aduth previously requested changes Jun 8, 2017
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Dunno if we might want to hold off on this in our push toward a stable release this week since it's not really fixing or adding anything.

},
wide: {
icon: 'align-full-width',
title: wp.i18n.__( 'Wide width' ),
Copy link
Member

Choose a reason for hiding this comment

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

Consistency: __

title: __( 'Align right' ),
},
wide: {
icon: 'align-full-width',
Copy link
Member

Choose a reason for hiding this comment

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

I think we want icon: 'align-wide'

@youknowriad youknowriad force-pushed the update/block-alignment-controls branch from 0a6f728 to 597af75 Compare June 8, 2017 15:26
@youknowriad
Copy link
Contributor Author

Ok to postpone :)

@youknowriad youknowriad force-pushed the update/block-alignment-controls branch from 597af75 to 705da6c Compare June 14, 2017 15:45
@youknowriad youknowriad dismissed aduth’s stale review June 14, 2017 15:48

Rebased, should be ok to review. Let's get this merged before the plugin release

@youknowriad youknowriad merged commit 106f41d into master Jun 19, 2017
@youknowriad youknowriad deleted the update/block-alignment-controls branch June 19, 2017 13:27
@youknowriad youknowriad mentioned this pull request Jun 22, 2017
3 tasks
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.

2 participants