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

Block Controls: Treat alignments toolbar as an inline toolbar #533

Merged
merged 1 commit into from
May 1, 2017

Conversation

youknowriad
Copy link
Contributor

As an alternative to #511, in this PR I'm treating the alignment controls as an inline toolbar (same as formats). Which means the state is inferred from the current selection. This allows per-block alignment inside blocks.

To opt-in to show the alignment toolbar, I've added a showAlignments prop to the Editable component.

@youknowriad youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 28, 2017
@youknowriad youknowriad self-assigned this Apr 28, 2017
@jasmussen
Copy link
Contributor

This seems to work great! 👍

@ellatrix
Copy link
Member

One concern I have with this is that it's not clear why what's being formatted is being formatted. Granted, this is a problem in the current editor as well, but I was hoping we could change this. It's neither applied to the selected block, nor the text selection.

@jasmussen
Copy link
Contributor

It's neither applied to the selected block, nor the text selection.

It's applied to the paragraph block, no?

I think the behavior of the current editor is okay on this front. If your cursor is inside an HTML element that has an alignment applied, the corresponding alignment button in the toolbar is pressed.

@youknowriad
Copy link
Contributor Author

It's neither applied to the selected block, nor the text selection.

Yeah! it's applied to the parent p of the selection which is understandable in my opinion, because we can not align an inline element inside a p.


Is this mergeable?

@jasmussen
Copy link
Contributor

Is this mergeable?

Barring any technical issues, I say go for it. And even with some technical issues, we can always revisit.

Though do remember to unwind in the weekend :)

@youknowriad youknowriad merged commit 928de66 into master May 1, 2017
@youknowriad youknowriad deleted the add/inline-alignments branch May 1, 2017 08:30
}

return result;
}, {} );
if ( tag === 'p' ) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason we restrict this to the p tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a restriction, that's where TinyMCE adds the alignment style. We can check in all the parent blocks, and then we'll have to decide for a priority (the closest maybe).

Copy link
Member

Choose a reason for hiding this comment

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

This is not a restriction, that's where TinyMCE adds the alignment style.

Ah. Thinking maybe this could be the forced_root_block setting:

https://www.tinymce.com/docs/configure/content-filtering/#forced_root_block

i.e. comparing to this.editor.settings.forced_root_block instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants