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: Clear floats on specific successive blocks that use alignment tool (Issue 13784) #13819

Closed
wants to merge 2 commits into from

Conversation

codetipi
Copy link

@codetipi codetipi commented Feb 11, 2019

Description

Fixes #13784

If you have two blocks that can be aligned (i.e. Image block), and the first block is aligned to the 'left', while the second is aligned to 'center' - The second block doesn't have clear: both; and therefore appears next to the floated left block.

Steps to reproduce the behavior:

  1. Add an Image block and align it to the left
  2. Duplicate the block and align this one to center

Expected behavior
The container of a block with an explicit center alignment should be full width and be cleared correctly, regardless of any blocks that exist before it.

Improves frontend/backend consistency of floats. Themes (Including Twentynineteen) generally clear blocks with center alignment if they come straight after a block with left/right float.

To recreate:

1- Add two blocks with alignment options, such as the Image block.
2- Set first block to align to left/right and set second block to align to center.
3- Second block should be 100% the width and clear the float from the first block.
@codetipi codetipi changed the title Fixes 13784 Fix: Clear floats on specific successive block (Issue 13784) Feb 11, 2019
@codetipi codetipi changed the title Fix: Clear floats on specific successive block (Issue 13784) Fix: Clear floats on specific successive blocks that use alignment tool (Issue 13784) Feb 11, 2019
@@ -305,6 +305,11 @@
border-bottom: $border-width solid $light-gray-500;
bottom: auto;
}

// Clear floats of centered block when previous sibling block has left/right float.
& + .editor-block-list__block[data-align="center"] {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not loaded in the frontend. Is this the expectation or are we expected to load a similar style in the frontend too?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah this is the expectation, as themes will most likely already have code to deal with this on the frontend. Here is TwentyNineteen's: https://github.com/WordPress/twentynineteen/blob/d6f9595f376f45d8ba0f203d9c3e3a393a4b3c18/sass/modules/_alignments.scss

That theme clears all blocks aligned to center:

.aligncenter { clear: both; display: block; margin-left: auto; margin-right: auto; }

But I wasn't sure if being this opinionated inside the editor was desired, which is why the PR only targets centered blocks that come after a left/right aligned block.

Do you think it should be more opinionated than that?

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds correct I think. In the editor, the center align has to be independent of any floats, otherwise it doesn't make much sense, since it will obey to its parent container anyway, if there is any advanced layout.

Currently it really makes no sense:

Screenshot 2019-04-22 at 21 31 16

@youknowriad youknowriad added the Needs Design Feedback Needs general design feedback. label Feb 12, 2019
@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 12, 2019
@gziolo gziolo added the General Interface Parts of the UI which don't fall neatly under other labels. label Feb 12, 2019
@gziolo gziolo added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Feb 25, 2019
@obenland obenland self-requested a review March 9, 2019 19:05
@@ -368,6 +369,11 @@
}
}

// Center
&[data-align="center"] {
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered doing &[data-align] [data-align="center"] to be a bit more specific to situations in where it's following another floated item?

Copy link
Contributor

@draganescu draganescu Apr 22, 2019

Choose a reason for hiding this comment

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

This is a good idea, however I would expect my block to not float no matter what it follows. @obenland do you have a specific example where this would be bad?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I have no idea what I meant there. I tested it locally and I think this is good to go.

@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
@mapk
Copy link
Contributor

mapk commented Apr 2, 2019

Expected behavior
The container of a block with an explicit center alignment should be full width and be cleared correctly, regardless of any blocks that exist before it.

This was discussed in the #design triage today and there was consensus around working toward the expected behavior.

@mapk mapk removed the Needs Design Feedback Needs general design feedback. label Apr 2, 2019
@gziolo gziolo added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jun 3, 2019
@obenland
Copy link
Member

@gziolo @noisysocks I can't resolve conflicts, otherwise I would have. That additional line can probably be removed in the process, but other than that I think this is a good improvement.

@gziolo
Copy link
Member

gziolo commented Jul 25, 2019

@gziolo @noisysocks I can't resolve conflicts, otherwise I would have. That additional line can probably be removed in the process, but other than that I think this is a good improvement.

There are two ways to do it. @codetipi does it or we create another branch which contains commits from the current one and it's rebased on top of the new branch.

@paaljoachim
Copy link
Contributor

Hey @codetipi

Could you give us a status update? Let us know if you need help or anything else to gain movement in this PR. Thanks.

@noisysocks noisysocks added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Aug 29, 2019
@noisysocks
Copy link
Member

There's some conflicts here. The branch will need to be rebased with the latest changes in master.

@talldan
Copy link
Contributor

talldan commented Aug 29, 2019

@noisysocks The code was moved to a separate PR:
#16780

I think it's probably worth closing this now as it's been quite a while without updates, despite a few pings (and it can always be reopened).

@talldan talldan closed this Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository General Interface Parts of the UI which don't fall neatly under other labels. Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Two blocks with align attribute are not being cleared correctly.
9 participants