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

RichText: Extend the AlignmentToolbar #14824

Closed
wants to merge 4 commits into from

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Apr 4, 2019

Description

Propose a way to extend the RichText alignment toolbar with additional options through a new registerCustomAlignmentType method that adds the option in state and adds it to the chosen block type's toolbar.

Note: I've yet to add a function to unregister custom alignment types.

How has this been tested?

⚠️ In 598b9d9 I've added a this registration in the editor initialization for testing purposes. Please drop the commit before merging. ⚠️

Create a plugin that enqueues this script:

wp.richText.registerCustomAlignmentType( 'test/custom-align-center', {
	align: 'center',
	icon: 'editor-aligncenter',
	title: 'Custom Align Center',
	blockName: 'core/paragraph',
} );

Open Gutenberg, insert a paragraph block and make sure that there is a new Center icon in the Alignment Toolbar, and that it works as expected.

Types of changes

New feature (non-breaking change which adds functionality)

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.

@Copons Copons added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] Rich text /packages/rich-text labels Apr 4, 2019
@Copons Copons self-assigned this Apr 4, 2019
@gwwar
Copy link
Contributor

gwwar commented Apr 4, 2019

Thanks @Copons! Would it be possible to split out the example to its own commit? This should make it easier to drop this later if this gets merged.

@gwwar
Copy link
Contributor

gwwar commented Apr 4, 2019

I took this out for an early spin, but didn't see an extra alignment control. Let us know how we can help, or give us a ping when this is ready!

*/
import { dispatch } from '@wordpress/data';

export function registerCustomAlignmentType( name, settings ) {
Copy link
Member

Choose a reason for hiding this comment

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

I know that this is a draft PR but I'd love to stress how valuable some inline documentation right here can be. people will look here to try and find the answer "what information is necessary to define an alignment type?" (if only we had some way to designate types and have the computer answer the question for us…)

looking through the whole PR I'm able to discern that it needs a name, a type, and control but I still don't know what those properties should be.

we could consider adding a JSDoc typedef

/**
 * @typedef {Object} AlignmentType
 * @property {string} type - blockType for which this will be available
 * @property {AlignmentControl} control - whatever this actually is, maybe another typedef
 */

/**
 * Registers a custom alignment type
 *
 * @example
 * registerCustomAlignmentType( 'justified', { type: 'core/paragraph', control: <Whatever /> } );
 *
 * @param {string} name - name of alignment, e.g. "left" or "justified"
 * @param {AlignmentType} settings - specifies the alignment
 */
export function registerCustomAlignmentType( name, settings ) {
  
}

even without the typedef though having an example use can clear up so much confusion, as can enumerating what properties of settings may exist and what they mean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out @dmsnell!

That file is a very stripped down version of register-format-type.js.
It doesn't have any docs, validations, etc, because this exploration is more about extending the toolbar than handling this registration.
I will add everything (and more!) back as soon as I get the extension part right. 👍

@Copons
Copy link
Contributor Author

Copons commented Apr 5, 2019

I took this out for an early spin, but didn't see an extra alignment control. Let us know how we can help, or give us a ping when this is ready!

@gwwar In 598b9d9 I've added the custom alignment registration in the editor init, so that it can be tested without having to create external plugins.

(Also, there were a few typos in the example code; I've fixed them now.)

@Copons Copons added the [Status] In Progress Tracking issues with work in progress label Apr 5, 2019
@Copons Copons marked this pull request as ready for review April 5, 2019 17:31
Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

@Copons Why is this added to the rich-text package? What does it have to do with rich text? Alignment is a block level setting?

@Copons
Copy link
Contributor Author

Copons commented Apr 5, 2019

Good point @ellatrix!
I've added this here because I've based part of the logic on registerFormatType, but you're right, it's definitely the wrong place.
Do you have any suggestions? There are several blocks-related packages, and I'm not familiar enough with them to know what would be the best choice.

dispatch( 'core/rich-text' ).addCustomAlignmentTypes( settings );

return settings;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd personally love us to move away from these register* global functions. That said, I'd like to understand the use-case a bit better. Why do you want a new alignment option, is this meant or custom blocks essentially or for Core Blocks.

Another downside of this approach is that if you disable the custom alignment, this might create invalid blocks in some cases (depending on how the alignment is applied to the block). And I think we should discourage the extensibility APIs that lead to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use-case would be to allow custom alignment options to be added to the alignment toolbar.
The most obvious one would be justify, which is a standard text-align value, so it should be straightforward enough to add back (with my implementation).
Less obvious ones would be additional block alignments like alignwide and alignfull.

Now, this is an exploratory PR opened by someone with close-to-zero familiarity with how extending Gutenberg works, so comments like yours about the risk of breaking a block are absolutely appreciated!
This said, I didn't expect to trigger a review request to seven different people when opening the PR, or otherwise I'd have tried to describe it a bit better, and clean it up a bit more 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

didn't expect to trigger a review request to seven different people when opening the PR

I guess, it means you're lucky to have early feedback.

About the justify alignment, I was thinking this is something we support by default "code-wise" but maybe just leave the UI hidden (it could be enabled using a shortcut). Is this a valid option?

Also, you're mixing both block and text alignment in the comments while it seems that at the moment, this PR is addressing text alignments only.

I wonder if this should be just another editor setting. The same way we have "colors" and "fontSizes" in the editor settings, we could include "blockAlignments" and "textAlignments". The block alignment option would be a little be redundunt with the "alignWide" setting but we can probably ensure BC. Thoughts?

@ellatrix
Copy link
Member

ellatrix commented Apr 6, 2019

@Copons Probably in the blocks package. :)

@gziolo gziolo added the [Feature] Extensibility The ability to extend blocks or the editing experience label Apr 8, 2019
@Copons
Copy link
Contributor Author

Copons commented Apr 10, 2019

Thanks for the reviews and suggestions, y'all!
I'm gonna close this PR and reopen another, putting the code in blocks, looking for another way to extend that doesn't use register* functions, etc. 🙂

@Copons Copons closed this Apr 10, 2019
@Copons Copons deleted the add/rich-text-alignment-extension branch April 10, 2019 11:36
@Copons Copons removed the [Status] In Progress Tracking issues with work in progress label Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] Rich text /packages/rich-text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants