WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 9 months ago

#23449 closed defect (bug) (fixed)

Refactor customizer accordion to work anywhere in admin

Reported by: lessbloat Owned by: markjaquith
Milestone: 3.6 Priority: normal
Severity: normal Version:
Component: Appearance Keywords: 3.6-menus has-patch commit
Focuses: Cc:

Description

We'll plan to use the default styles/structure from the customizer accordion for menu item selection options on nav-menus.php. Currently the code is very customizer specific.

  • The class names should be made more generic.
  • The CSS should be extracted and placed in wp-admin.css
  • The JS, should be moved so that you don't have to include the customizer JS elsewhere in the admin to re-use the accordion code.

Attachments (26)

23449.diff (18.4 KB) - added by lessbloat 14 months ago.
23449.1.diff (18.3 KB) - added by lessbloat 14 months ago.
23449.2.diff (18.5 KB) - added by aaroncampbell 14 months ago.
23449.3.diff (5.7 KB) - added by lessbloat 13 months ago.
23449.png (26.4 KB) - added by ocean90 13 months ago.
23449.4.diff (6.7 KB) - added by lessbloat 13 months ago.
23449.rtl.diff (1.3 KB) - added by SergeyBiryukov 13 months ago.
23449.5.diff (5.7 KB) - added by JustinSainton 13 months ago.
Refresh
23449.6.diff (3.4 KB) - added by lessbloat 9 months ago.
23449.7.diff (4.8 KB) - added by lessbloat 9 months ago.
23449.8.diff (5.0 KB) - added by lessbloat 9 months ago.
23449.9.diff (5.0 KB) - added by helen 9 months ago.
23449.10.diff (5.3 KB) - added by aaroncampbell 9 months ago.
23449.12.diff (5.3 KB) - added by aaroncampbell 9 months ago.
23449.11.diff (5.5 KB) - added by aaroncampbell 9 months ago.
23449.13.diff (5.6 KB) - added by DrewAPicture 9 months ago.
vars
23449.14.diff (5.9 KB) - added by DrewAPicture 9 months ago.
RTL fix for the arrows
23449.15.diff (1.5 KB) - added by ocean90 9 months ago.
23449.15b.diff (1.8 KB) - added by ocean90 9 months ago.
Changes also the cursor
23449.16.diff (2.2 KB) - added by helen 9 months ago.
23449.17.diff (3.0 KB) - added by ocean90 9 months ago.
23449.18.diff (3.7 KB) - added by ocean90 9 months ago.
Support for a 'closable' class, which can be used for the Customizer to let theme description be closable
23449.19.diff (3.7 KB) - added by ocean90 9 months ago.
23449.20.diff (7.0 KB) - added by helen 9 months ago.
23449.21.diff (8.3 KB) - added by helen 9 months ago.
23449.22.diff (8.5 KB) - added by helen 9 months ago.

Download all attachments as: .zip

Change History (70)

comment:1 helen14 months ago

  • Keywords ui-focus added

comment:2 DrewAPicture14 months ago

  • Cc xoodrew@… added

comment:3 sabreuse14 months ago

  • Cc sabreuse added

lessbloat14 months ago

comment:4 lessbloat14 months ago

I mistakenly uploaded this to #23450 yesterday. Oops...

23449.diff​ is a first pass at this.

The basic reusable structure looks like this:

<div class="accordion-container">
	<ul>
		<li class="control-section accordion-section" id="accordion-section-title_tagline">
			<h3 title="" tabindex="0" class="accordion-section-title">Title here</h3>
			<ul class="accordion-section-content">
				<li><!-- content here --></li>
			</ul>
		</li>
	</ul>
</div>

So you'd enqueue the new "accordion" JS file, and drop this code anywhere in the admin.

A couple notes

  • I left all of the color CSS in wp-admin.css, since for now both fresh, and classic use the same colors for the accordion.
  • We could optionally move all of the form element styling (used within the customizer) over. I left that out of this first pass.
  • .accordion-section-content, could just as well be a DIV if you only have one block of code to add there (which is likely what I'll do for menus).
Last edited 14 months ago by lessbloat (previous) (diff)

comment:5 DrewAPicture14 months ago

Copying my comment from #23450:

23449.diff shows no noticeable differences, so that's good.

One question I have is whether we're:

A) Breaking this out so that it can be reused in core only

B) Breaking this out for core + public use?

Whichever way we go, we should probably at the very least lay out basic usage in a docblock somewhere (maybe accordion.js)

comment:6 follow-up: aaroncampbell14 months ago

I think the patch looks fine. It's basically moving working code to a new location without changing anything (except for generalizing some class names).

Having said that, I'd like to fix some things that prevent them from being nested because someone is going to try to nest them (you know they will).

That means using closest() instead of parents(), changing $( '.accordion-section' ).not( clicked ).removeClass( 'open' ); to clicked.closest( '.accordion-container' ).find( '.accordion-section' ).not( clicked ).removeClass( 'open' );, and changing all uses of id to class.

lessbloat14 months ago

comment:7 in reply to: ↑ 6 lessbloat14 months ago

Replying to aaroncampbell:

I'd like to fix some things that prevent them from being nested because someone is going to try to nest them (you know they will).

That means using closest() instead of parents(), changing $( '.accordion-section' ).not( clicked ).removeClass( 'open' ); to clicked.closest( '.accordion-container' ).find( '.accordion-section' ).not( clicked ).removeClass( 'open' );, and changing all uses of id to class.

Good call. 23449.1.diff​ should take care of those items. I just pulled the "Scroll up if on #accordion-section-title_tagline" code altogether as it's too specific to this one implementation, and to be honest, I don't think it improves the experience in any way.

Last edited 14 months ago by lessbloat (previous) (diff)

aaroncampbell14 months ago

comment:8 aaroncampbell14 months ago

23449.2.diff adds the accordion-container class to the customizer accordion. Without it sections don't close when you open a new one.

comment:9 jkudish14 months ago

  • Cc jkudish added

comment:10 markjaquith14 months ago

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

In 23417:

Refactor the Customizer accordion so that it can be used in other locations.

fixes #23449. props lessbloat, aaroncampbell

comment:11 markjaquith13 months ago

In 23707:

Turn the Nav Menu meta boxes into an accordion. Less sprawling and overwhelming.

  • Registration stays the same — they're meta boxes
  • Call do_accordion_sections() instead of do_meta_boxes() and they render as an accordion

props DrewAPicture, lessbloat, jkudish. fixes #23450. see #23449

comment:12 markjaquith13 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

DrewAPicture did a ton more work on this in #23450. The new strategy is that accordions are just a meta box section that is rendered differently. That way we can keep using our same meta box API.

But I don't think we're 100% there. As a test, I went into edit-form-advanced.php and tried turning my sidebar into an accordion. The sections wouldn't open when clicked. Is there some part of the JS that is not 100% abstracted?

lessbloat13 months ago

comment:13 lessbloat13 months ago

23449.3.diff​ does a few things:

  • Enqueues accordion.js inside do_accordion_sections()
  • Removes triplicate JS that was in accordion.js (what happened there? ;-))
  • Overrides a couple of styles specifically for using an accordion on post-new.php

comment:14 GaryJ13 months ago

  • Cc gary@… added

ocean9013 months ago

comment:15 follow-up: ocean9013 months ago

23449.png - The top border radius should be removed.

lessbloat13 months ago

comment:16 in reply to: ↑ 15 lessbloat13 months ago

Replying to ocean90:

The top border radius should be removed.

23449.4.diff​ should fix that, plus refreshes the patch.

comment:17 lessbloat13 months ago

  • Keywords has-patch commit added; needs-patch ui-focus removed

comment:18 SergeyBiryukov13 months ago

In [23417], .customize-section was replaced with .accordion-section.

Looks like RTL styles could use an update (see also #23740):
http://core.trac.wordpress.org/browser/trunk/wp-admin/css/customize-controls-rtl.css?rev=23417#L21

SergeyBiryukov13 months ago

comment:19 SergeyBiryukov13 months ago

In 23784:

Update RTL styles for Customizer. see #23449.

comment:20 lessbloat13 months ago

  • Keywords has-patch commit removed
  • Resolution set to fixed
  • Status changed from reopened to closed

From what I can tell this is all done.

comment:21 SergeyBiryukov13 months ago

  • Keywords has-patch needs-refresh added
  • Resolution fixed deleted
  • Status changed from closed to reopened

23449.4.diff still seems valid. Needs a refresh.

JustinSainton13 months ago

Refresh

comment:22 JustinSainton13 months ago

  • Keywords needs-refresh removed

Refreshed 23449.4.diff.

comment:23 follow-up: nacin13 months ago

I can't quite tell how much code is duplicated between do_accordion_sections() and do_meta_boxes(), but it seems like quite a bit. (And this code is, shall we say, unique, so keeping it DRY has some importance.) Could we just add an $args array to do_meta_boxes() for 'accordion' => true?

comment:24 in reply to: ↑ 23 DrewAPicture13 months ago

Replying to nacin:

I can't quite tell how much code is duplicated between do_accordion_sections() and do_meta_boxes(), but it seems like quite a bit. (And this code is, shall we say, unique, so keeping it DRY has some importance.) Could we just add an $args array to do_meta_boxes() for 'accordion' => true?

Mind continuing this discussion over in #23450 instead? This ticket's more specific to the refactored js, though they're obviously linked.

comment:25 nacin10 months ago

Is 23449.5.diff still needed? Drew, Dave, Mark, Sergey?

comment:26 follow-up: lessbloat9 months ago

I'd say 23449.6.diff is worth including.

  • Moving wp_enqueue_script( 'accordion' ); makes it's usage more universal.
  • The rounded corners are specific to the accordion inside #nav-menu-meta
  • The "#submitdiv", and "#poststuff" code should likely go in a separate ticket, as they only apply when you (optionally) turn the postbox's on the post edit page into an accordion.

Note: I added the JS changes in there too, since moving them makes them more universal as well.

Last edited 9 months ago by lessbloat (previous) (diff)

lessbloat9 months ago

comment:27 helen9 months ago

I don't think it's necessary to check if the script is already enqueued, but other than that this seems good. Good call on taking the #submitdiv and #poststuff part out of this patch.

comment:28 follow-up: helen9 months ago

I take it back - didn't test thoroughly enough. The initial opening does not work, and because sections can be hidden but are in the source, the CSS selectors for first/last-child don't really work if first/last are hidden. It's especially bad if the first is hidden.

lessbloat9 months ago

comment:29 in reply to: ↑ 28 ; follow-up: lessbloat9 months ago

Replying to helen:

I take it back - didn't test thoroughly enough. The initial opening does not work,

Ah, right, sorry about that. Should work in .7.

and because sections can be hidden but are in the source, the CSS selectors for first/last-child don't really work if first/last are hidden. It's especially bad if the first is hidden.

filter( ':visible' ) should take care of that, no?

comment:30 in reply to: ↑ 29 lessbloat9 months ago

Replying to lessbloat:

and because sections can be hidden but are in the source, the CSS selectors for first/last-child don't really work if first/last are hidden. It's especially bad if the first is hidden.

filter( ':visible' ) should take care of that, no?

Man, I'm rusty... Sorry, totally spaced the whole "CSS selectors" part there. I'll take another look.

lessbloat9 months ago

comment:31 lessbloat9 months ago

Refactored soe of the JS in 23449.8.diff​. Adds .top and .bottom classes now via JS to handle the rounded corners.

helen9 months ago

aaroncampbell9 months ago

comment:32 aaroncampbell9 months ago

23449.10.diff just adds a couple styles to Helen's most recent patch to account for no-js. Since sections can't be hidden with no-js it just uses :first-child and :last-child

aaroncampbell9 months ago

aaroncampbell9 months ago

comment:33 aaroncampbell9 months ago

Well, looks like Drew removed his patch, so the most recent is 23449.11.diff not .12. It fixes the display issue with rounded corners when the bottom section is collapsed and also hides the arrows from no-js

DrewAPicture9 months ago

vars

comment:34 DrewAPicture9 months ago

23449.13.diff moves accordionOptions out of accordionCorners() so we can reuse the cached var in accordionInit(). Also created sectionContent to tighten it up a bit.

DrewAPicture9 months ago

RTL fix for the arrows

comment:35 helen9 months ago

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

In 24680:

Tweaks to the accordion:

  • Enqueues JS in do_accordion_sections().
  • Top and bottom rounded corners for the nav menu accordion.
  • Better RTL and no-JS.

props lessbloat, DrewAPicture, aaroncampbell, helen. fixes #23449.

comment:36 in reply to: ↑ 26 helen9 months ago

Replying to lessbloat:

  • The "#submitdiv", and "#poststuff" code should likely go in a separate ticket, as they only apply when you (optionally) turn the postbox's on the post edit page into an accordion.

Mind making a new ticket? Or whoever else can get to it before I can.

ocean909 months ago

comment:37 ocean909 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

There shouldn't be an animation when you click on a section which is already open. 23449.15.diff solves that.

ocean909 months ago

Changes also the cursor

helen9 months ago

comment:39 helen9 months ago

Two more things in the customizer:

  • Hover/open styling for the section headers is broken. Fixed in 23449.16.diff.
  • Theme details (first section) no longer opens.

comment:40 helen9 months ago

Also, a note that we may want to tone down the open section header for places not the customizer so as not to confuse what's primary and what's not. Can make a separate ticket for this.

ocean909 months ago

ocean909 months ago

Support for a 'closable' class, which can be used for the Customizer to let theme description be closable

ocean909 months ago

helen9 months ago

helen9 months ago

comment:41 helen9 months ago

Per IRC, we are going to flip it so that sections can always be closed and leave anything that needs to be added/reinstated for one-section-always-open for later, as in not this cycle. The first visible section will continue to be loaded open. The part where the first visible section is re-opened when a section is hidden via screen option will probably also go away - as it is, it's too aggressive, as at most it should only happen if the hidden section was open when hidden.

Last edited 9 months ago by helen (previous) (diff)

helen9 months ago

comment:42 nacin9 months ago

  • Keywords commit added

Looks and works well to me.

comment:43 DrewAPicture9 months ago

After some discussion in IRC, I can see muting the contrast in the menus accordion styles because it (unfortunately) helps bring it better in line with other dashboard screens. And we're all about consistency here :)

In lieu of a better alternative, +1 to 23449.22.diff. We'll probably have to revisit this when we tackle mp6 anyway.

Accordion before: http://cl.ly/image/0A0I180X0k39
Accordion after: http://cl.ly/image/1p3b2o2V1q0u

comment:44 markjaquith9 months ago

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

In 24734:

Fixes and tweaks for the metabox accordion.

  • Fixed hover issues in Customizer.
  • Toned down the default active/hover style (as it is normally not the top level menu, and should not be so prominent).
  • Allow the active section to be closed.
  • Other misc fixes.

Props ocean90, helen. Fixes #23449.

Note: See TracTickets for help on using tickets.