WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years 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: Customize 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 3 years ago.
23449.1.diff (18.3 KB) - added by lessbloat 3 years ago.
23449.2.diff (18.5 KB) - added by aaroncampbell 3 years ago.
23449.3.diff (5.7 KB) - added by lessbloat 2 years ago.
23449.png (26.4 KB) - added by ocean90 2 years ago.
23449.4.diff (6.7 KB) - added by lessbloat 2 years ago.
23449.rtl.diff (1.3 KB) - added by SergeyBiryukov 2 years ago.
23449.5.diff (5.7 KB) - added by JustinSainton 2 years ago.
Refresh
23449.6.diff (3.4 KB) - added by lessbloat 2 years ago.
23449.7.diff (4.8 KB) - added by lessbloat 2 years ago.
23449.8.diff (5.0 KB) - added by lessbloat 2 years ago.
23449.9.diff (5.0 KB) - added by helen 2 years ago.
23449.10.diff (5.3 KB) - added by aaroncampbell 2 years ago.
23449.12.diff (5.3 KB) - added by aaroncampbell 2 years ago.
23449.11.diff (5.5 KB) - added by aaroncampbell 2 years ago.
23449.13.diff (5.6 KB) - added by DrewAPicture 2 years ago.
vars
23449.14.diff (5.9 KB) - added by DrewAPicture 2 years ago.
RTL fix for the arrows
23449.15.diff (1.5 KB) - added by ocean90 2 years ago.
23449.15b.diff (1.8 KB) - added by ocean90 2 years ago.
Changes also the cursor
23449.16.diff (2.2 KB) - added by helen 2 years ago.
23449.17.diff (3.0 KB) - added by ocean90 2 years ago.
23449.18.diff (3.7 KB) - added by ocean90 2 years 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 2 years ago.
23449.20.diff (7.0 KB) - added by helen 2 years ago.
23449.21.diff (8.3 KB) - added by helen 2 years ago.
23449.22.diff (8.5 KB) - added by helen 2 years ago.

Download all attachments as: .zip

Change History (70)

comment:1 @helen3 years ago

  • Keywords ui-focus added

comment:2 @DrewAPicture3 years ago

  • Cc xoodrew@… added

comment:3 @sabreuse3 years ago

  • Cc sabreuse added

@lessbloat3 years ago

comment:4 @lessbloat3 years 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 3 years ago by lessbloat (previous) (diff)

comment:5 @DrewAPicture3 years 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: @aaroncampbell3 years 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.

@lessbloat3 years ago

comment:7 in reply to: ↑ 6 @lessbloat3 years 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.

Version 1, edited 3 years ago by lessbloat (previous) (next) (diff)

@aaroncampbell3 years ago

comment:8 @aaroncampbell3 years 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 @jkudish3 years ago

  • Cc jkudish added

comment:10 @markjaquith3 years 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 @markjaquith2 years 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 @markjaquith2 years 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?

@lessbloat2 years ago

comment:13 @lessbloat2 years 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 @GaryJ2 years ago

  • Cc gary@… added

@ocean902 years ago

comment:15 follow-up: @ocean902 years ago

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

@lessbloat2 years ago

comment:16 in reply to: ↑ 15 @lessbloat2 years ago

Replying to ocean90:

The top border radius should be removed.

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

comment:17 @lessbloat2 years ago

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

comment:18 @SergeyBiryukov2 years 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

@SergeyBiryukov2 years ago

comment:19 @SergeyBiryukov2 years ago

In 23784:

Update RTL styles for Customizer. see #23449.

comment:20 @lessbloat2 years 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 @SergeyBiryukov2 years 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.

@JustinSainton2 years ago

Refresh

comment:22 @JustinSainton2 years ago

  • Keywords needs-refresh removed

Refreshed 23449.4.diff.

comment:23 follow-up: @nacin2 years 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 @DrewAPicture2 years 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 @nacin2 years ago

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

comment:26 follow-up: @lessbloat2 years 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 2 years ago by lessbloat (previous) (diff)

@lessbloat2 years ago

comment:27 @helen2 years 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: @helen2 years 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.

@lessbloat2 years ago

comment:29 in reply to: ↑ 28 ; follow-up: @lessbloat2 years 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 @lessbloat2 years 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.

@lessbloat2 years ago

comment:31 @lessbloat2 years ago

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

@helen2 years ago

@aaroncampbell2 years ago

comment:32 @aaroncampbell2 years 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

@aaroncampbell2 years ago

@aaroncampbell2 years ago

comment:33 @aaroncampbell2 years 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

@DrewAPicture2 years ago

vars

comment:34 @DrewAPicture2 years 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.

@DrewAPicture2 years ago

RTL fix for the arrows

comment:35 @helen2 years 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 @helen2 years 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.

@ocean902 years ago

comment:37 @ocean902 years 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.

@ocean902 years ago

Changes also the cursor

@helen2 years ago

comment:39 @helen2 years 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 @helen2 years 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.

@ocean902 years ago

@ocean902 years ago

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

@ocean902 years ago

@helen2 years ago

@helen2 years ago

comment:41 @helen2 years 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 2 years ago by helen (previous) (diff)

@helen2 years ago

comment:42 @nacin2 years ago

  • Keywords commit added

Looks and works well to me.

comment:43 @DrewAPicture2 years 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 @markjaquith2 years 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.