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: Add direction attribute / LTR button to paragraph block #10663

Merged
merged 1 commit into from Oct 18, 2018

Conversation

Projects
None yet
4 participants
@aduth
Member

aduth commented Oct 16, 2018

Closes #138

This pull request seeks to add an LTR toggle button to the paragraph block toolbar when an RTL locale is active.

image

It adds a new direction block to the paragraph block. As implemented, the value will only ever be ltr or undefined, given current UI behavior as toggle only shown when the editor is configured as RTL. When non-undefined, it assigns a dir attribute to the saved markup. This should be entirely backward-compatible with existing paragraph blocks.

Future compatibility notes:

  • Directionality could be abstracted as a common blocks feature, similar to align, color, etc.

Testing Instructions:

Verify there's no LTR button when an LTR locale is active.

Verify that an LTR button is active and serves as a toggle for paragraph block when an RTL locale is active.

cc @yoavf @aahani

@aduth aduth added the Blocks label Oct 16, 2018

@aduth aduth added this to the 4.1 - UI freeze milestone Oct 16, 2018

@aduth aduth requested a review from youknowriad Oct 16, 2018

@@ -65,6 +65,10 @@ const schema = {
customFontSize: {
type: 'number',
},
direction: {
type: 'string',
enum: [ 'ltr', 'rtl' ],

This comment has been minimized.

@youknowriad

youknowriad Oct 16, 2018

Contributor

do we support this enum property?

This comment has been minimized.

@aduth

aduth Oct 16, 2018

Member

Not yet 😉

This comment has been minimized.

@aduth

aduth Oct 16, 2018

Member

Well, to clarify, it's supported in the server-side validation, but isn't ported to the client. Idea with adding here now is it "just works" when ported.

This comment has been minimized.

@youknowriad

youknowriad Oct 16, 2018

Contributor

There's probably a lot of block attributes where this could be considered. align for instance

This comment has been minimized.

@aduth
@youknowriad

The approach looks good to me. I wonder if we should add an RTL e2e test (not just specific to this though). I'd appreciate a review from @yoavf as he has more experience on these matters.

@aduth aduth referenced this pull request Oct 16, 2018

Closed

Add support for RTL scripts #138

controls={ [
{
icon: 'editor-ltr',
title: _x( 'Left to right', 'editor button' ),

This comment has been minimized.

@aduth

aduth Oct 16, 2018

Member

In case this raises eyebrows, the hope / intention is to inherit the existing string:

https://github.com/WordPress/WordPress/blob/da7a80d67fea29c2badfc538bfc01c8a585f0cbe/wp-includes/class-wp-editor.php#L1128

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 18, 2018

Let's merge it as is and revisit later if @yoavf has some concerns.

@gziolo gziolo merged commit 70583ac into master Oct 18, 2018

2 checks passed

codecov/project 49.49% (+<.01%) compared to 87d67ee
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the add/p-direction branch Oct 18, 2018

@mtias mtias added the i18n label Oct 18, 2018

antpb added a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment