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

Fixed RichText placeholder whitespace. #5035

Merged
merged 1 commit into from
Feb 15, 2018

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Feb 13, 2018

The RichText CSS uses a rule based on attribute data-is-placeholder-visible. The TinyMCE component was just setting this attribute when componentWillReceiveProps is invoked while it should also set it after the first mount for the first props received, after TinyMCE setup is complete. So the attribute was missing while the component did not receive new props. This PR fixes that bug.

How Has This Been Tested?

Add an image block with an image, focus the title of the post then click on the image. Verify no empty space appears between the image and the placeholder. It may not be easy to replicate the bug, trying with a small 300x300 image made things easier to replicate.
Verify other uses cases of RichText and see no regression happens.

Create a new post verify there is no big whitespace in the paragraph that disappears, on mouse hover.

Screenshots (jpeg or gifs if applicable):

feb-13-2018 15-53-11
feb-13-2018 15-52-06

@jorgefilipecosta jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Feb 13, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Feb 13, 2018
@jorgefilipecosta jorgefilipecosta changed the title Fixed RichText placeholder whitespace. In progress - Fixed RichText placeholder whitespace. Feb 13, 2018
@jorgefilipecosta jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Feb 13, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the fix/empty-whitespace-placeholder branch 2 times, most recently from 642d8df to 5b52913 Compare February 13, 2018 16:41
@jorgefilipecosta jorgefilipecosta changed the title In progress - Fixed RichText placeholder whitespace. Fixed RichText placeholder whitespace. Feb 13, 2018
@jorgefilipecosta jorgefilipecosta removed the [Status] In Progress Tracking issues with work in progress label Feb 13, 2018
@aduth
Copy link
Member

aduth commented Feb 14, 2018

I think the original issue can also be seen when saving a post with empty paragraphs and refreshing? There's additional space around the empty blocks until cursor moves over them. Initial testing reveals this is improved by this branch. Still a brief flicker of height-changing.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/empty-whitespace-placeholder branch from 5b52913 to a4a935c Compare February 14, 2018 15:40
The RichText CSS uses a rule based on attribute data-is-placeholder-visible. The TinyMce component was just setting this attribute when componentWillReceiveProps is invoked while it should also set on the first render for the first props received.
@jorgefilipecosta jorgefilipecosta force-pushed the fix/empty-whitespace-placeholder branch from a4a935c to 5f34816 Compare February 14, 2018 15:45
@jorgefilipecosta
Copy link
Member Author

Still a brief flicker of height-changing.

Hi @aduth, nice catch code was updated instead of using mount event we are now setting the attribute directly on render. The attribute should now immediately appear.

@ellatrix
Copy link
Member

When was this introduced?

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Feb 14, 2018

When was this introduced?

Not certain but it seems to be a very old problem, even before the attribute was named data-is-placeholder-visible we used data-is-empty and it looks like in that attribute data-is-empty we also had that problem. So it does not seems like a regression, probably the problem become more noticeable recently because of some design change.

@ellatrix
Copy link
Member

Yeah, I've seen this bug only just recently, so it just seemed like a regression.

@aduth aduth added this to the 2.2 milestone Feb 14, 2018
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

To clarify, the only effective change here is including the data- prop in the render result?

I could see how this might have always been an issue, if we'd relied on the fact that componentWillReceiveProps would always have been called to "correct" the appearance (might also explain why it's intermittent).

Makes sense to me 👍

@jorgefilipecosta
Copy link
Member Author

To clarify, the only effective change here is including the data- prop in the render result?

Yes that's the only effective change. The refactor to an external function of the placeholder in componentWillReceiveProps was part of a different approach to solving this problem. And as the change was done when reverting I kept it because it makes sense.
The other changes are updates to test snapshots.

@jorgefilipecosta jorgefilipecosta merged commit ad182a5 into master Feb 15, 2018
@jorgefilipecosta jorgefilipecosta deleted the fix/empty-whitespace-placeholder branch February 15, 2018 13:04
@ellatrix
Copy link
Member

Thanks for these changes @jorgefilipecosta!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants