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

Block: fix alignments #19704

Merged
merged 6 commits into from
Jan 20, 2020
Merged

Block: fix alignments #19704

merged 6 commits into from
Jan 20, 2020

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jan 16, 2020

Description

After #19593, this PR reintroduces a div wrapper element for blocks that support alignment. This wrapper div is necessary to contain the aligned block within the main block column. Maybe in the future, the block itself can add this wrapper div, but for now we have to provide it.

Note that the wrapper div has the is-block-content class. Regardless of how we rewrite alignments, an attribute like this will be necessary so a block can decide where the toolbar should be attached to.

How has this been tested?

Test alignments on blocks.

Screenshots

Screenshot 2020-01-16 at 16 02 09

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

Co-Authored-By: Chris Van Patten <hello@chrisvanpatten.com>

return <Component { ...props } className={ className } />;
if ( attributes.align ) {
return <div className="is-block-content">{ component }</div>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add it in block.js ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could add it there as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :)

@jasmussen
Copy link
Contributor

I don't believe how fast you fixed this. It appears to work just as it should, even for things like embeds. So just on the experience, 👍 👍! Impressive work.

I noticed you fixed it using a is-block-content class that presumably helps keep it in the "main column" — is this an interim fix?

@ellatrix
Copy link
Member Author

I noticed you fixed it using a is-block-content class that presumably helps keep it in the "main column" — is this an interim fix?

I'm not sure. I think eventually we'll probably see something similar, because, while the block could provide this wrapper in the future, the toolbar will still have to be positioned at this wrapper instead of the parent element (which spreads the whole length). I could see this as something provided by us to the block though. Maybe with a future <Alignment> component that the block could use around the content, and we handle the rest.

@jasmussen
Copy link
Contributor

I'm not sure. I think eventually we'll probably see something similar, because, while the block could provide this wrapper in the future, the toolbar will still have to be positioned at this wrapper instead of the parent element (which spreads the whole length).

Excellent point. Floats are the worst.

I could see this as something provided by us to the block though. Maybe with a future <Alignment> component that the block could use around the content, and we handle the rest.

I was specifically thinking of how the Image block currently receives an extra wrapping div when floated — with a meta goal of getting the editor closer to the frontend in markup, leveraging that could be a piece of the puzzle.

But did not mean to suggest the current solution isn't good — it seems totally fine. You've eliminated so many divs, it's fine to add one back :D

@ellatrix
Copy link
Member Author

Yes, right now, the extra div is needed, but soon we'll be able to remove one more for image (the outer one). worth noting that this adds a div for blocks with alignment but not for the others. :)


// For aligned blocks, provide a wrapper element so the block can be
// positioned relative to the block column.
if ( attributes.align ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this is a good way to check whether a block has alignment. For me a more precise way is to check the wrapper props and see if there's a "data-align" because that's how floating is triggered.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. I changed it to data-align.

@ellatrix ellatrix merged commit 8af69dd into master Jan 20, 2020
@ellatrix ellatrix deleted the fix/alignments branch January 20, 2020 08:49
@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
@ellatrix ellatrix added the [Type] Regression Related to a regression in the latest release label Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants