WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#35220 closed defect (bug) (fixed)

collapsed Customize sidebar , showing empty while we change page view from desktop to responsive mobile view

Reported by: mostafas Owned by: afercia
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Customize Keywords: has-patch has-screenshots commit
Focuses: ui Cc:

Description

Hello
I'm new in bug report.and don't know exactly this is a bug or somethings else.
Well...
While i try to test new WordPress 4.5 trunk version , going to customize sidebar, after that i collapse customize sidebar and use responsive design mode on Firefox and resize windows width to 460px(in fact 640x502) and customize collapsed sidebar will expand with no text on sidebar.

I also test on chrome, clear Firefox cache.but problem yet exist.it this ok...or i maybe shouldn't ever do that like this.

Attachments (6)

Screen Shot 2015-12-25 at 11.55.32.png (7.4 KB) - added by mostafas 5 years ago.
problem on firefox on responsive designe mode at 640x502
better-screenshot.png (31.8 KB) - added by mostafas 5 years ago.
Better Screenshot
35220.diff (428 bytes) - added by rittesh.patel 5 years ago.
Patch for #35220
35220.2.diff (869 bytes) - added by rittesh.patel 5 years ago.
Add missing rtl CSS fix.
35220.3.diff (554 bytes) - added by afercia 5 years ago.
35220.4.diff (553 bytes) - added by afercia 5 years ago.

Download all attachments as: .zip

Change History (23)

@mostafas
5 years ago

problem on firefox on responsive designe mode at 640x502

@mostafas
5 years ago

Better Screenshot

#1 @afercia
5 years ago

  • Focuses administration removed
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.5
  • Version changed from trunk to 4.4

At a screen width of 640 pixels and below the Customizer two columns layout:

https://cldup.com/tKkpZ4Ad8b.png

switches to a "responsive view" with a one column layout:

https://cldup.com/zw3q_dD8s0.png

where the sidebar has a width of 100%, the "Collapse" button disappears, and there's a new UI control to switch from the Customizer sidebar to the preview.

By the way, when the sidebar is collapsed and then the screen width becomes less or equal to 640 pixels (for example users may change orientation on a mobile device) the sidebar is still collapsed and this rule in themes.css will still set a negative left margin on the sidebar so the sidebar text will be off screen:

.wp-full-overlay.collapsed .wp-full-overlay-sidebar {
	margin-left: -300px;
}

This rule should be simply reset to margin-left: 0; in a CSS media query. By the way I'm not sure why it's in themes.css in the first place. Any feedback welcome :)

Please consider also that in the Theme installer the UI works differently and there's a bit of inconsistency: there's no responsive view and the layout is always a two columns layout. Maybe it should work in the same way as the Customizer?

https://cldup.com/N4_4tjOfCO.png

#2 @westonruter
5 years ago

@jtsternberg This is closely related to the work you've been doing on the resizer.

#3 @mostafas
5 years ago

I found the solution , but i dont know how to fix it

when screen width is reach 640px , then add expanded class to main customize div
i mean <div class="wp-full-overlay expanded"></div>
.expanded{
margin-left; 300px;
}

but how to do this what css i must edit? i see that css load from load-styles.php file

I think this bug dont happen for usual users...just happen if a users have a tablet and change orientation of tablet while she/he on previous orientation change customize sidebar to collapsed and now in new orientation it encounter this bug

@rittesh.patel
5 years ago

Patch for #35220

#5 @rittesh.patel
5 years ago

  • Keywords has-patch added; needs-patch removed

In 35220.diff instead of .wp-full-overlay-sidebar css selector, #customize-controls is used as to increase the element specificity.

@rittesh.patel
5 years ago

Add missing rtl CSS fix.

#6 @westonruter
5 years ago

@rittesh.patel FYI: the RTL CSS is automatically generated as part of the build, along with the minified CSS.

#7 @rittesh.patel
5 years ago

Oh, didn't know that, it must have been added recently. Thanks @westonruter for pointing out. If that's the case we can ignore 35220.2.diff and consider 35220.diff.

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


5 years ago

#9 @westonruter
5 years ago

  • Owner set to afercia
  • Status changed from new to reviewing

Please review.

#10 @danlouw
5 years ago

Hi @rittesh.patel and @afercia ,

I had a look at this as well and as far as I could tell the patch fixes the issue from a functional point of view, but I was wondering about what the intended behaviour should be.

If the Customizer sidebar was collapsed in the two-column layout (width of more than 640px), should the sidebar not remain out of view when the layout converts to one-column when re-sizing to 640px or less?

In other words the page preview should be displayed and “Customize” UI control should appear, instead of the Customiser sidebar re-appearing to fill the whole one-column layout, shouldn’t it?

#11 @rittesh.patel
5 years ago

Hi @danlouw

I think your point makes sense and the behavior would be to show the Customizer in Preview mode and let the sidebar remain collapsed. If user clicks on Customize (width is 640px or less) than Customize sidebar will be shown and if layout is re-sized and width is greater than 640px then we can consider the sidebar to be expanded.

@afercia
5 years ago

#12 @afercia
5 years ago

  • Keywords has-screenshots added

@danlouw @rittesh.patel yeah, "keeping the current state" when switching to the responsive view and vice-versa would make sense. I think it can't be done with CSS only though and would require some more investigation. At this point of the release (we're in Beta 2) I'd propose to go for a simple fix and improve in future iterations.

@westonruter refreshed the patch just to rebuild it from the root and added a comment. Looks good to me.

#13 follow-up: @afercia
5 years ago

Side note: wondering if we should move this rule to customize-controls.css (also there's already a media query at 640px there).

#14 in reply to: ↑ 13 ; follow-up: @westonruter
5 years ago

Replying to afercia:

Side note: wondering if we should move this rule to customize-controls.css (also there's already a media query at 640px there).

Makes sense to me. Would you like to make the change?

@afercia
5 years ago

#15 in reply to: ↑ 14 @afercia
5 years ago

Replying to westonruter:

Makes sense to me. Would you like to make the change?

There you go :) See 35220.4.diff

Thinking at the idea about "keeping the current state", maybe when under 640px the .collapsed view (screenshot on the left) should be the same as the .preview-only view (screenshot on the right). They're not exactly the same thing though, see the top toolbar. Maybe something worth considering for the next release cycle?

https://cldup.com/iJZlp_WbVR.png

#16 @westonruter
5 years ago

  • Keywords commit added

@afercia looks good. I think you should commit. @mostafas if you could confirm that the patch fixes the issue as you experienced it as well, that would be great.

#17 @afercia
5 years ago

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

In 36877:

Customizer: Improve the collapsed view when switching to the responsive view.

Props rittesh.patel, afercia.

Fixes #35220.

Note: See TracTickets for help on using tickets.