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

Fix regression with appender. #9782

Merged
merged 2 commits into from
Sep 11, 2018
Merged

Fix regression with appender. #9782

merged 2 commits into from
Sep 11, 2018

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Sep 11, 2018

This PR is a hotfix for a small regression introduced in #9735. Basically the empty "Write your story" placeholder had no top margin, and jumped when clicked. This PR fixes that.

Before:

before

After:

after

Additionally it fixes an issue I think might have regressed with editor styles. The appender height used to be the same as a paragraph height (28px, 56px with margins). But this regressed. This PR adds an explicit height to the appender input field to fix this, noting that a Paragraph is actually a paragraph, whereas the appender is an input field.

height

☝️ in that GIF I type in "Write your story", then delete the block, then use undo/redo to ensure they look identical

This PR is a hotfix for a small regression introduced in #9735. Basically the empty "Write your story" placeholder had no top margin, and jumped when clicked. This PR fixes that.
@jasmussen jasmussen added [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release labels Sep 11, 2018
@jasmussen jasmussen added this to the 3.9 milestone Sep 11, 2018
@jasmussen jasmussen self-assigned this Sep 11, 2018
@jasmussen jasmussen requested review from kjellr and a team September 11, 2018 07:38
@jasmussen
Copy link
Contributor Author

Also fixed it for the inbetween appender:

inbetween

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

✅ Looks good to me. No more jumping cursor. 👍

@jasmussen
Copy link
Contributor Author

Awesome, thanks so much. Can you approve the PR?

@jasmussen jasmussen requested a review from a team September 11, 2018 12:40
Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

👏

@jasmussen jasmussen merged commit e7f2803 into master Sep 11, 2018
@jasmussen jasmussen deleted the fix/appender branch September 11, 2018 12:58
@websevendev
Copy link

Hello,

is this working as intended?

Previously a nested block had a bunch of extra margin above and below (which I could collapse but I don't mind them since it makes the editor experience easier), but was positioned somewhat realistically compared to the front end. Now, how ever, it has less margin on top and more on bottom which is a completely inaccurate representation:

Although that's not a big deal, since the block appenders tend to also leave a lot of trailing space that's not actually there on the front end.

But also take a look at this use case:

I've got these nested column blocks in a grid block, .editor-block-list__layout is set to display: flex; flex-direction: row; and the first column is now offset by the reduction of first-child margin:

I guess I can manually set a fixed margin, but honestly I think they should be consistent out of the box. I thought .editor-block-list__layout is designed to allow rearranging blocks the way you need them with flex or even CSS grid, so currently it feels a bit inconsistent that the first child margins are altered.

Thanks!

@jasmussen
Copy link
Contributor Author

Thanks for the report, @websevendev that looks like a regression. Not of this PR, but a related one. I will take a look.

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 [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants