Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#39349 closed defect (bug) (fixed)

Customizer (mobile preview) site title extra padding

Reported by: halftones's profile halftones Owned by: westonruter's profile westonruter
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch commit fixed-major
Focuses: Cc:

Description

Customizer adds extra 10px left padding to site title when preview in mobile mode.

Attachments (2)

Screen Shot 2016-12-21 at 00.42.19.png (67.7 KB) - added by halftones 8 years ago.
39349.diff (661 bytes) - added by sirbrillig 8 years ago.
Remove global title/widget padding added when edit shortcuts are visible

Download all attachments as: .zip

Change History (15)

#1 @westonruter
8 years ago

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

@halftones thanks for the report. This is to accommodate the visual edit shortcuts. If your theme implemented selective refresh to preview changes to the title then you'd see an edit shortcut there in the preview. Note also that if you collapse the controls pane the padding should be removed, since the edit shortcuts are also then removed.

@sirbrillig Should we extend the base Partial JS logic to add a class name to all containers for partial placements? And then this class name can be used in the selectors to add this additional padding.

#2 @halftones
8 years ago

My theme not using selective refresh, and i don't see any edit shortcuts. I just see this padding and only in mobile mode.

#3 @sirbrillig
8 years ago

@westonruter Oh, I didn't realize there was body padding being added. Yeah, the shortcuts definitely shouldn't affect the page layout if at all possible, and certainly not if they're not even present. (At the very least I think that sort of global adjustment styling should be up to the theme.)

Adding a class name to partial containers is a clever solution that sounds like it would prevent this padding from being applied without a partial.

I wonder, however, if the padding is needed at all. A quick test of a few default themes without that padding looks ok to my eye (although we'd definitely have to do a more careful analysis to see what that was meant to fix).

#4 @westonruter
8 years ago

@sirbrillig didn't you add the padding so that the edit shortcuts wouldn't get cut off at the edge of the screen?

#5 @sirbrillig
8 years ago

@westonruter it's... possible? But I *think* that was added by someone else. There were a lot of contributions to the CSS of that patch. I've never been particularly concerned about the icons being cut off. I think it's a better experience than changing the layout to accommodate them. Regardless of who wrote it originally, I don't think it's a good solution.

#6 @westonruter
8 years ago

@sirbrillig see #38651.

#7 @sirbrillig
8 years ago

@westonruter nice find! :-) Looks like I copy-and-pasted those rules from this patch https://core.trac.wordpress.org/attachment/ticket/38651/38651.5.diff into the next one without looking very carefully.

I still think it's not wise for the edit shortcut styles to move anything in the preview if there's any way to prevent it.

I'll drop a patch here to strip those styles out and we can see what it looks like.

@sirbrillig
8 years ago

Remove global title/widget padding added when edit shortcuts are visible

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


8 years ago

#9 @adamsilverstein
8 years ago

  • Keywords has-patch needs-screenshots added; needs-patch removed

#10 @celloexpressions
8 years ago

  • Focuses performance removed
  • Keywords commit added; needs-screenshots removed

The customizer should not be modifying any theme styles directly; any compensating for edit shortcuts needs to happen on those elements or within themes themselves.

+1 for 39349.diff, which should be in 4.7.1 if possible and looks ready for commit. Removing needs-screenshots since the "before" screenshot clearly illustrates the padding that's being removed by the patch.

#11 @westonruter
8 years ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from new to closed

In 39685:

Customize: Remove extra left padding in core for site title and widgets in preview.

These styles should only be applied by theme stylesheets as needed.

Props sirbrillig.
Reverts parts of [39202] and [39233].
See #38651.
Fixes #39349.

#12 @westonruter
8 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for 4.7.1

#13 @dd32
8 years ago

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

In 39695:

Customize: Remove extra left padding in core for site title and widgets in preview.

These styles should only be applied by theme stylesheets as needed.

Props sirbrillig.
Reverts parts of [39202] and [39233].
See #38651.
Merges [39685] to the 4.7 branch.
Fixes #39349.

Note: See TracTickets for help on using tickets.