Make WordPress Core

Opened 11 years ago

Closed 9 years ago

Last modified 8 years ago

#26396 closed enhancement (fixed)

Implement a generic clearfix

Reported by: helen's profile helen Owned by: afercia's profile afercia
Milestone: 4.5 Priority: low
Severity: minor Version: 4.4
Component: Administration Keywords: has-patch deprecated-css
Focuses: ui Cc:

Description

We have a clearfix defined in core, but it's applied to very specific items. It should be a generic class. We also have an old-school br.clear and a .clear class - should look for those and replace them in core wherever it makes sense.

Attachments (7)

26396.patch (451 bytes) - added by afercia 9 years ago.
26396.2.patch (829 bytes) - added by afercia 9 years ago.
26396.3.patch (6.6 KB) - added by afercia 9 years ago.
26396.4.patch (7.1 KB) - added by afercia 9 years ago.
26396.5.remove-old-clearfix.patch (15.2 KB) - added by afercia 9 years ago.
26396.6.remove-old-clearfix.patch (15.3 KB) - added by afercia 9 years ago.
26396.7.remove-old-clearfix.patch (14.1 KB) - added by afercia 9 years ago.

Download all attachments as: .zip

Change History (37)

This ticket was mentioned in Slack in #core by valendesigns. View the logs.


10 years ago

#2 @afercia
10 years ago

  • Focuses ui administration added
  • Keywords ui-fous removed

+1 for this.
A more modern method to contain floated elements would be very useful especially now that core is using a focus style with box-shadows. In many places they're "broken" cause overflow: hidden used to contain floats. See for example: #29897 and #26646. Would suggest:

.contain-floats:after {
    content: '';
    display: table;
    clear: both;
}

Naming convention to discuss of course.

#3 @joemcgill
10 years ago

For reference: Chris Coyier has kept a pretty good running diary of the various CSS patterns for this here: https://css-tricks.com/snippets/css/clear-fix/

#4 @afercia
10 years ago

thanks @joemcgill. Had a second thought about naming convention, I was wrong. With overflow: hidden still in my mind I was thinking to "contain floats" because that's what overflow: hidden actually does, but with the pseudo-element it's really a "clearfix" :)

#5 @wonderboymusic
9 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

I would love for someone to pick this up - there are divs floating in the breeze all over the place that have no layout, yet contain untold numbers of floated elements

#6 @afercia
9 years ago

Related #28864 among other things tries to introduce a generic rule for this in common.css:

.contain-floats:after {
	content: '';
	display: table;
	clear: both;
}

#7 @afercia
9 years ago

  • Focuses administration removed
  • Milestone changed from Future Release to 4.5
  • Owner set to afercia
  • Status changed from new to assigned
  • Version set to 4.4

The original clearfix mentioned in the ticket description is now in nav-menus.css. On the other hand, [35673] introduced a new rule in common.css, currently used only for the "navigation tabs" container:

/* contain floats */
.nav-tab-wrapper:after {
	content: "";
	display: table;
	clear: both;
}

I'd suggest to just add a generic class name, maybe "wp-clearfix"?

/* modern clearfix */
.wp-clearfix:after,
.nav-tab-wrapper:after {
	content: "";
	display: table;
	clear: both;
}

cc @michaelarestad cause I know he's passionate about CSS naming conventions :)

This ticket was mentioned in Slack in #design by afercia. View the logs.


9 years ago

#9 follow-up: @michaelarestad
9 years ago

@afercia You know me so well. I agree that .wp-clearfix is an ideal class name. Once in place, we could remove the need for .nav-tab-wrapper and any other classes using the rules for a clearfix.

@afercia
9 years ago

#10 in reply to: ↑ 9 @afercia
9 years ago

Replying to michaelarestad:

we could remove the need for .nav-tab-wrapper and any other classes using the rules for a clearfix.

Thinking probably it's not so easy. For example:

  • plugins already use the "Tabs" so removing the clearfix from .nav-tab-wrapper will break their layout since it's unlikely plugins authors are going to add .wp-clearfix
  • stuff that can be enqueued in the frontend (e.g. Editor, Media Modals) would need a clearfix rule in a file that's available in the frontend

I'd say that for now the most important thing is introducing the new clearfix and use it for new developments. See proposed patch. Any thoughts more than welcome.

#11 follow-up: @michaelarestad
9 years ago

I'd say that for now the most important thing is introducing the new clearfix and use it for new developments. See proposed patch. Any thoughts more than welcome.

Ah. I agree with that. The patch looks good, though makes me a bit sad that we need to keep the extra class in there.

#12 in reply to: ↑ 11 @afercia
9 years ago

Replying to michaelarestad:

though makes me a bit sad that we need to keep the extra class in there.

Yep :( maybe one more reason to seriously consider @helen 's idea to introduce a deprecated.css

@afercia
9 years ago

#13 @afercia
9 years ago

The refreshed patch moves the new clearfix rule to the "utility classes" section.

@afercia
9 years ago

#14 follow-up: @afercia
9 years ago

Thinking it would be better to start using the new CSS class in the cleanest possible way so what about something like 26396.3.patch where:

  • Tabs output by core will use wp-clearfix
  • Tabs output by plugins will fallback to a slightly less-ideal clearfix method overflow: hidden

The CSS would be cleaner and the back-compatibility rules ready to be moved in a deprecated file. Any thoughts more than welcome.

#15 in reply to: ↑ 14 ; follow-up: @michaelarestad
9 years ago

Replying to afercia:

Thinking it would be better to start using the new CSS class in the cleanest possible way so what about something like 26396.3.patch where:

  • Tabs output by core will use wp-clearfix
  • Tabs output by plugins will fallback to a slightly less-ideal clearfix method overflow: hidden

I generally stay away from overflow: hidden because it could have unexpected results. I'm not sure we get a clear benefit from doing this other than completely separating the clearfix. My preference would be to either duplicate the clearfix to keep the clearfix class separate from other classes or just go with something like 26396.2.patch.

I lean toward the duplication of rules. I'm also wondering if we should start separating out utility classes like wp-clearfix from common.css into their own file.

#16 in reply to: ↑ 15 @afercia
9 years ago

Replying to michaelarestad:

I lean toward the duplication of rules.

Thanks Michael. OK, wiill refresh the patch.

I'm also wondering if we should start separating out utility classes like wp-clearfix from common.css into their own file.

Good point :)

@afercia
9 years ago

#17 @afercia
9 years ago

  • Keywords has-patch added; needs-patch removed

OK so in order to keep the clearfix class separate from other classes and at the same time be as clean as possible and preserve back-compat I'd propose:

  • for modern browsers, use .nav-tab-wrapper:not(.wp-clearfix):after
  • for IE 8, no need to be 100% clean so I'd propose to always establish a new block formatting context

See refreshed patch.

Open to suggestions and improvements. Can also be refined later. For now, I'd propose to commit what we have and move on since this is blocking other tickets (see for example #28864).

Will keep this ticket open for further patches to replace the old clearfix and all the br.clear and div.clear. Patches welcome :)

#18 @afercia
9 years ago

In 36171:

Introduce a new generic CSS clearfix utility class.

.wp-clearfix is now the recommended way to clear and contain floated elements.
Adds back compatibility for the .nav-tab-wrapper navigation tabs.

See #26396.

#19 @afercia
9 years ago

Patch 26396.5.remove-old-clearfix.patch is first attempt to remove the old clearfix applied to specific items across the admin. Some testing would be nice :)

Version 0, edited 9 years ago by afercia (next)

#20 @paulwilde
9 years ago

@afercia <div id="widgets-right wp-clearfix"> should be <div id="widgets-right" class="wp-clearfix"> in src/wp-admin/customize.php.

Last edited 9 years ago by paulwilde (previous) (diff)

#21 @afercia
9 years ago

@paulwilde thanks! Patch refreshed.

This ticket was mentioned in Slack in #design by afercia. View the logs.


9 years ago

#23 @afercia
9 years ago

Refreshed patch after recent changes to the markup. Patches like this one tend to stale quickly so I'd propose to commit now and if something breaks I think there's enough time to fix it at this point of the release cycle. By the way, looks good to me.

#24 @afercia
9 years ago

In 36422:

After [36171] remove all the occurrences of the old CSS clearfix.

The old clearfix was applied to very specific items and defined multiple times
across CSS files. Uses the new generic .wp-clearfix utility class instead.

See #26396.

#25 follow-up: @lkraav
9 years ago

What's the ticket for dropping the legacy box model flow breaking floating strategy and moving to modern grid approach like flexbox instead?

#26 in reply to: ↑ 25 @afercia
9 years ago

Replying to lkraav:

What's the ticket for dropping the legacy box model flow breaking floating strategy and moving to modern grid approach like flexbox instead?

@lkraav there isn't one, as far as I know. Please do feel free to open a new ticket if you think starting a discussion about flexbox is worthy. I'm afraid there's no consensus about the adoption of the flexbox model for core, yet. Also, core still supports IE 8 and 9.

This ticket was mentioned in Slack in #core by afercia. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by jorbin. View the logs.


9 years ago

#29 @jorbin
9 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

Calling this complete for 4.5. If there are other changes needed in 4.5, we can reopen it as a task.

#30 @afercia
8 years ago

  • Keywords deprecated-css added
Note: See TracTickets for help on using tickets.