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 floats in a full-bleed world #799

Closed
wants to merge 3 commits into from
Closed

Conversation

jasmussen
Copy link
Contributor

This PR seeks to address #784 by applying an invisible and calculated outer margin to floats. This margin just happens to match the space surrounding the 700px max-width of the visual editor, making it look right.

I realize the calc looks advanced, but it really simplifies the responsive CSS.

@jasmussen jasmussen self-assigned this May 16, 2017
@jasmussen jasmussen requested review from youknowriad and aduth May 16, 2017 09:28
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I don't know if it's related but I'm still seeing an issue with floated blocks if they precede an embed block


@include break-medium() {
margin-left: calc( 50% - #{ $visual-editor-max-width / 2 } );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Aside: Do we expect block authors to implement this CSS styles (which are not that easy) in each floated block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good aside. And no, I've been thinking of it this way:

  • Full-width can be implemented in many different ways, we've chosen one of many ways
  • Theme authors should probably opt-in to the feature, probably something like add_theme_support( 'full-width-images' );
  • When the theme has opted in, images simply get align-full-width as a CSS class. Or something in that vein.

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 asking about block authors too. I mean, should we move these styles outside the image block stylesheet and provide a way to reuse it across different blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, good point. Taking a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we move the visual styles out as part of this PR, or as part of for example #729? Tackling that one seems like a good successor to this one, as video should also have full width as an option.

@youknowriad
Copy link
Contributor

Here is the undesired behaviour

screen shot 2017-05-16 at 10 34 09

@jasmussen
Copy link
Contributor Author

Thanks for the screenshot, taking a look!

This seems like the right thing to do, because videos normally have an intrinsice width and height, and so they would clear floats naturally just like how an image following a floating would. But because we are using CSS trickery to make the video responsive, we are essentially pulling the video out of the flow, which means it's technically abs positioned. By adding a clear, we emulate the behavior. The edgecases where the clear for an embed would be undesired are arguably edgecases.
@jasmussen
Copy link
Contributor Author

Upon discussing this with @mtias I've decided to close this branch, go back to the max-width, then take a pass at using JS for full-width images.

@jasmussen jasmussen closed this May 16, 2017
@jasmussen jasmussen deleted the fix/full-bleed-floats branch May 16, 2017 12:43
jasmussen added a commit that referenced this pull request May 16, 2017
This also adds a few improvements from the now deleted branch, #799, notably:

- clearing for video blocks. The responsive trickery means we have to, or we will get odd behavior otherwise
- better collapsing margins fix for the toolbar.
mtias pushed a commit that referenced this pull request May 23, 2017
This also adds a few improvements from the now deleted branch, #799, notably:

- clearing for video blocks. The responsive trickery means we have to, or we will get odd behavior otherwise
- better collapsing margins fix for the toolbar.
mtias pushed a commit that referenced this pull request Jun 2, 2017
This also adds a few improvements from the now deleted branch, #799, notably:

- clearing for video blocks. The responsive trickery means we have to, or we will get odd behavior otherwise
- better collapsing margins fix for the toolbar.
mtias pushed a commit that referenced this pull request Jun 2, 2017
This also adds a few improvements from the now deleted branch, #799, notably:

- clearing for video blocks. The responsive trickery means we have to, or we will get odd behavior otherwise
- better collapsing margins fix for the toolbar.
mtias pushed a commit that referenced this pull request Jun 7, 2017
This also adds a few improvements from the now deleted branch, #799, notably:

- clearing for video blocks. The responsive trickery means we have to, or we will get odd behavior otherwise
- better collapsing margins fix for the toolbar.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants