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

Only apply appender margins when the appender is inside of a block #15888

Merged
merged 2 commits into from May 30, 2019

Conversation

@kjellr
Copy link
Contributor

commented May 29, 2019

As pointed out by @jasmussen in #15868, the default appender for Twenty Nineteen appears improperly indented until it's clicked. After some investigation, I've determined that this is a side effect of the new(ish) margins added to the default appender in #14241.

In most themes, these margins are collapsed, so this has no effect. But they're visible in Twenty Nineteen, and push the placeholder text over:

Screen Shot 2019-05-29 at 8 57 15 AM

This PR increases specificity of those margins so that they are put in place only when the default appender exists within an actual block. When it's outside of a block, those margins are unnecessary (I think?). This fixes the problem in Twenty Nineteen, and has no negative effects in any other theme I've tried.


Before:

Twenty Nineteen:

Screen Shot 2019-05-29 at 8 50 45 AM

Gutenberg Starter Theme (Appears as expected):

Screen Shot 2019-05-29 at 9 02 58 AM

After:

Twenty Nineteen:

Screen Shot 2019-05-29 at 8 51 06 AM

Gutenberg Starter Theme (No change):

Screen Shot 2019-05-29 at 9 02 32 AM

@kjellr kjellr requested a review from jasmussen May 29, 2019

@kjellr kjellr self-assigned this May 29, 2019

@kjellr kjellr changed the title Only apply appender margins when the appender exists within a block Only apply appender margins when the appender is inside of a block May 29, 2019

@talldan
Copy link
Contributor

left a comment

I wonder how this issue regressed 🤔

The fix works well, and it'd be nice to get it merged, as its one of those things that's a fairly apparent visual issue—something that many will notice during their first experience of the editor.

Only other thought is that it might be worth adding a comment explaining the additional class.

@kjellr

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

Only other thought is that it might be worth adding a comment explaining the additional class.

Thanks, @talldan! I've added an explainer in f74480e. I'll merge this once the tests pass.

@kjellr kjellr merged commit 2d541eb into master May 30, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@kjellr kjellr deleted the fix/block-appender-position branch May 30, 2019

@youknowriad youknowriad added this to the Gutenberg 5.9 milestone Jun 7, 2019

nicolad added a commit to nicolad/gutenberg that referenced this pull request Jun 15, 2019

Only apply appender margins when the appender is inside of a block (W…
…ordPress#15888)

* Only apply appender margins when the appender exists within a block.

* Add code comment to explain the extra specificity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.