Skip to content

Conversation

@jasmussen
Copy link
Contributor

This addresses feedback in #11757 (comment).

The initial issue was that in the default appender, and in empty paragraphs, the plus to add blocks overlaps the placeholder text. The right-padding fixed that. But it bled into other blocks with placeholders, such as the Button block.

This PR scopes the rich text rule to only apply to empty paragraphs. The appender was already scoped.

Before:

before

After:

after

The button isn't as wide on the right side, in the after.

This addresses feedback in #11757 (comment).

The initial issue was that in the default appender, and in empty paragraphs, the plus to add blocks overlaps the placeholder text. The right-padding fixed that. But it bled into other blocks with placeholders, such as the Button block.

This PR scopes the rich text rule to only apply to empty paragraphs. The appender was already scoped.
@jasmussen jasmussen added the [Type] Enhancement A suggestion for improvement. label Nov 20, 2018
@jasmussen jasmussen added this to the 4.6 milestone Nov 20, 2018
@jasmussen jasmussen self-assigned this Nov 20, 2018
@jasmussen jasmussen requested a review from a team November 20, 2018 13:37
// On mobile and in nested contexts, the plus to add blocks shows up on the right.
// This padding makes sure it doesn't overlap text.
& + .editor-rich-text__tinymce {
& + .editor-rich-text__tinymce.wp-block-paragraph {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that we target specific blocks in a generic component, what's the reason here? We could do the opposite if needed. Target the richText in the block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Target the richText in the block.

Do you mean move these styles into the paragraph block?

Given we have a unique case for the empty paragraph block, I think it's okay to make rules specific to that. But you could have a point in where the styles should live.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, move to paragraph what's specific to paragraph and leave here what's generic to RichText.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thoughts. Fixed it in 0879d8a:

screenshot 2018-11-20 at 15 35 46

// Specific to the empty paragraph placeholder:
// when shown on mobile and in nested contexts, the plus to add blocks shows up on the right.
// This padding makes sure it doesn't overlap text.
.editor-rich-text__tinymce[data-is-placeholder-visible="true"] + .editor-rich-text__tinymce.wp-block-paragraph {
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be in editor.scss right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh, yes it should. Many apologies, I know you're busy. 81a81a4 should fix that up.

@catehstn
Copy link

@youknowriad looks like your comments have been addressed -- can we get this merged?

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.

LGTM 👍

@jasmussen jasmussen merged commit 17deea5 into master Nov 21, 2018
@jasmussen jasmussen deleted the fix/nested-appender-regression branch November 21, 2018 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants