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

Fix: CSS rule affecting nested paragraphs placeholder #10197

Merged
merged 1 commit into from Oct 4, 2018

Conversation

Projects
None yet
3 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Sep 26, 2018

Description

We were forcing a height of 28px for paragraph placeholders but the paragraph may have a huge font size so 28px may not be enough for the placeholder.
This PR replaces the usage of height with min-height in the problematic rule.

I guess this was a recent regression because I noticed this bug after rebasing #9416.

How has this been tested?

I checked the following test block appears as expected https://gist.github.com/jorgefilipecosta/7f0a54193f9d8d28c1514aa2e57a00fc.

Screenshots

Before:
screen shot 2018-09-26 at 16 44 45

After:
screen shot 2018-09-26 at 16 44 04

@jorgefilipecosta jorgefilipecosta requested a review from jasmussen Sep 27, 2018

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core Oct 4, 2018

@tofumatt

tofumatt requested changes Oct 4, 2018 edited

Seems like the new selector is redundant. Maybe I'm missing something?

I'm not clear on how to test this without checking out that other branch–is there an easier way to test this locally? Scratch that, I'm dumb, and I see now. I opened the linked PR and the Gist in tabs and confused them. My bad! 😓

@@ -114,10 +114,14 @@
}
}
// Ensure that the height of the first appender, and the one between blocks, is the same as text.
.editor-block-list__block[data-type="core/paragraph"] p[data-is-placeholder-visible="true"] + p,

This comment has been minimized.

@tofumatt

tofumatt Oct 4, 2018

Member

Why is this a new/repeated selector? It seems the same as the one below.

@tofumatt

tofumatt Oct 4, 2018

Member

Why is this a new/repeated selector? It seems the same as the one below.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Oct 4, 2018

Member

You are right, the selector is repeated, during my tries to solve this I tried other approach and then I missed to simplify this PR. It was simplified and now the selector is not repeated.
Thank you for catching this 👍

@jorgefilipecosta

jorgefilipecosta Oct 4, 2018

Member

You are right, the selector is repeated, during my tries to solve this I tried other approach and then I missed to simplify this PR. It was simplified and now the selector is not repeated.
Thank you for catching this 👍

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 4, 2018

Contributor

Yep! If tofumatts comments are addressed, ship it!

Contributor

jasmussen commented Oct 4, 2018

Yep! If tofumatts comments are addressed, ship it!

@tofumatt

Tested locally and it's nice! 🚢

@jorgefilipecosta jorgefilipecosta merged commit 579f584 into master Oct 4, 2018

2 checks passed

codecov/project 49.21% remains the same compared to 37bd1e6
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/css-rule-affecting-nested-paragraph-placeholder branch Oct 4, 2018

@jorgefilipecosta jorgefilipecosta added this to the 4.0 milestone Oct 8, 2018

@jorgefilipecosta jorgefilipecosta self-assigned this Oct 8, 2018

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