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

Adjust margin rules for nested blocks. #9735

Merged
merged 1 commit into from Sep 10, 2018

Conversation

Projects
None yet
3 participants
@jasmussen
Contributor

jasmussen commented Sep 10, 2018

This PR one one hand fixes #9730, and on the other hand simplifies some of the column wrangling code to be simpler.

It does visualy regress it slightly: previously we'd use negative margins to make it so text inside columns looked to be spaced the same as text before and after the columns.

However this greatly complexified the CSS, which caused #9730 in the first place. The thing is — the columns block itself has a margin. This margin collapses correctly to adjacent blocks. But given it also has children, those margins don't also collapse. Possibly we could make this happen, I'm unsure, flex containers are supposed to prevent margin collapsing by creating their own contexts, but it seems like the negative margins added complexity where it wasn't helpful.

Before:

screen shot 2018-09-10 at 12 38 17

After:

screen shot 2018-09-10 at 12 33 20

The difference is subtle. But look how the margin between text inside and outside the columns block used to be the same. This is no longer the case, but perhaps worth it as a simplification fix, and then possibly something to revisit separately.

Adjust margin rules for nested blocks.
This PR one one hand fixes #9730, and on the other hand simplifies some of the column wrangling code to be simpler.

It does visualy regress it slightly: previously we'd use negative margins to make it so text inside columns looked to be spaced the same as text before and after the columns.

However this greatly complexified the CSS, which caused #9730 in the first place. The thing is — the columns block itself has a margin. This margin collapses correctly to adjacent blocks. But given it also has children, those margins don't also collapse. Possibly we could make this happen, I'm unsure, flex containers are supposed to prevent margin collapsing by creating their own contexts, but it seems like the negative margins added complexity where it wasn't helpful.

@jasmussen jasmussen self-assigned this Sep 10, 2018

@jasmussen jasmussen requested review from youknowriad and aduth Sep 10, 2018

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 10, 2018

Contributor

Another great one… really loving all these simplifications to the editor CSS!

Contributor

chrisvanpatten commented Sep 10, 2018

Another great one… really loving all these simplifications to the editor CSS!

@youknowriad

LGTM 👍

@jasmussen jasmussen added this to the 3.9 milestone Sep 10, 2018

@jasmussen jasmussen merged commit aa39621 into master Sep 10, 2018

2 checks passed

codecov/project 49.03% remains the same compared to 410f78a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the try/alternate-nested-margins-fix branch Sep 10, 2018

jasmussen added a commit that referenced this pull request Sep 11, 2018

Fix regression with appender.
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 added a commit that referenced this pull request Sep 11, 2018

Fix regression with appender. (#9782)
* Fix regression with appender.

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.

* Add comments, and fix for inbetween appender also.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment