Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trac 37661 redux #216

Closed
wants to merge 82 commits into from

Conversation

Projects
None yet
3 participants
@westonruter
Copy link
Collaborator

commented Feb 13, 2017

@westonruter westonruter changed the title Trac 37661 redux [WIP] Trac 37661 redux Feb 13, 2017

celloexpressions and others added some commits Feb 15, 2017

Remove favorites functionality
This feature is most useful for people that install the same themes on
numerous sites; that is rarely the case for themes, unlike with plugins.
Remove featured view
Featured themes on .org are actually random. It is more helpful to show
latest or popular themes with the option of filtering them.
Remove latest and popular sections
These are actually intended to be used as sort order options. Latest
themes will be displayed by default when there are no filters.
Remove API for text before a section heading
Intentionally preserves the CSS for this in a more generic way that can
be used elsewhere by core an third-parties. This is designed to provide
the correct spacing and alignment between sections when including
additional text.
Merge search and feature filter sections into wporg section
First pass at combining filtering sections and moving filters into the
preview area. Brings the installed theme filter to all users. Needs
styling and testing.
Initial styling for the filter bar and feature filter
Restores existing core feature filter styling.
Rework feature filters in core and dettach from .org API
Place subject filters first, then features, then layouts. Aggressively
remove tags from the list that do not provide significant value to the
average users. Reasons for removal include: tags that apply to most
themes, tags that apply to few enough themes that (roughly) more than
half of the results are themes abusing the tag list that should not have
the tag, tags that are confusing or unclear to a typical user, tags for
features that most themes provide. Detach the .org API from the theme
list and only use the list provided in core; we can reconcile the core
and .org feature lists the next time the .org directory is reworked.
Push theme grid below feature filter when expanded.
The Filter Themes dropdown should push the themes down, not overlay
them. This to ensure that the themes are visible while the filters
update, also to allow the users to interact with the full form without
closing it every time just to see the first entry.

Implementation is relatively complex because the filter bar is fixed on
larger screens, so we can't do it entirely with CSS.
Improve filter bar right alignment with theme grid
Unfortunately it is not possible to align in all cases because there can
be a scrollbar next to the themes but not the filter bar on some screen
sizes, and the scrollbar width varies by browser/device.
Improve filter bar on various screen sizes
Down to the 640px mobile breakpoint where the sidebar is dropped.
Preserve search term across section changes
Also abstracts the filter-search functionality to its own function.
@@ -5399,6 +6082,10 @@

// Sort the sections within each panel
api.panel.each( function ( panel ) {
if ( 'themes' === panel.id ) {
return; // Don't reflow theme sections, as doing so moves them after the themes container.

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 29, 2017

Author Collaborator

Hard-coding this seems not ideal.

if ( otherSection !== section ) {

// Try to sync the current search term to the new section.
if ( 'themes' === otherSection.params.type ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 29, 2017

Author Collaborator

Hard-coding a themes condition in here is not ideal.

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

Why not other than the variable name can be themeSection to be more specific ? The other way can be to just let other sections collapse in this loop and write the themeSection separately themeSection = api.section( 'installed_themes' ).

section.container.find( noFilter ).hide();
}

renderScreenshots = _.throttle( _.bind( section.renderScreenshots, this ), 100 );

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 29, 2017

Author Collaborator

This doesn't make sense to me. It's throttling the calls to renderScreenshots but it is then renderScreenshots is just called once below. Should not the renderScreenshots function be wrapped in _.throttle() where it is defined below?

@@ -6440,6 +7209,18 @@
// Collapse the most granular expanded object.
collapsedObject = expandedControls[0] || expandedSections[0] || expandedPanels[0];
if ( collapsedObject ) {
if ( 'themes' === collapsedObject.params.type ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 29, 2017

Author Collaborator

Hard-coding this here isn't right. It should be added to an overridden collapse method on wp.customize.ThemesPanel.

@sayedtaqui

This comment has been minimized.

Copy link

commented Sep 29, 2017

@westonruter If I preview some theme, I get the following error

image

}

if ( section.term === newTerm ) {
return;

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

The return here is unnecessary. We can do

if ( section.term !== newTerm ) {
 section.initializeNewQuery( newTerm, section.tags );
}

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

Or even better, remove the above return too, and just do

if ( 'wporg' === section.params.action ) {
	newTerm = $( '#wp-filter-search-input' ).val();
} else if ( section.term !== newTerm ) {
	section.initializeNewQuery( newTerm, section.tags );
}

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 29, 2017

Author Collaborator

Not quite, I think it should be:

		checkTerm: function( section ) {
			var newTerm;
			if ( 'wporg' === section.params.action ) {
				newTerm = $( '#wp-filter-search-input' ).val();
				if ( section.term !== newTerm ) {
					section.initializeNewQuery( newTerm, section.tags );
				}
			}
		},

// Check whether tags have changed, and either load or queue them.
if ( section.tags === tags ) {
return;

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

unnecessary return

@@ -1820,10 +2279,11 @@
*
* @since 4.2.0
*
* @param {Object} theme
* @param {Object} theme - Theme.

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

Missing callback definition.

@@ -1441,20 +1496,28 @@

// Pressing the escape key fires a theme:collapse event
if ( 27 === event.keyCode ) {
section.closeDetails();
if ( $( 'body' ).hasClass( 'modal-open' ) ) {

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

We are using $( 'body' ) at a lot of places, why dont we cache it.

*
* @returns {void}
*/
initializeNewQuery: function( newTerm, newTags ) {

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

Missing param definitions.

embed: function() {
var inject,
section = this,
container = $( '#customize-theme-controls' );

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

We now have this available in section.containerParent

},

/**
* Return the section's content element without detachng from the parent.

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

Typo "detachng"

installTheme: function( event ) {
var panel = this, preview = false, slug = $( event.target ).data( 'slug' );

if ( -1 !== $.inArray( this.installingThemes, slug ) ) {

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

underscore has a cleaner way of checking this condition _.contains( this.installingThemes, slug )

});

// Don't add the same theme more than once.
if ( ! theme || 'undefined' !== typeof api.control( 'installed_theme_' + theme.id ) ) {

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

Wouldn't 'undefined' !== typeof api.control( 'installed_theme_' + theme.id ) be equal to api.control.has( 'installed_theme_' + theme.id ) ?

// Update loading message. Everything else is handled by reloading the page.
$( '#customize-themes-loading-container span' ).hide();
$( '#customize-themes-loading-container .customize-loading-text' ).css( 'display', 'block' );
wp.a11y.speak( $( '#customize-themes-loading-container .customize-loading-text' ).text() );

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

Lets cache $( '#customize-themes-loading-container' ) and then use .find( 'span' ) to get
these elements?

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 29, 2017

Author Collaborator

Since this query is only going to happen once in a given Customizer session, I don't think this is necessary.

*
* @since 4.9.0
*
* @param {jQuery.Event} Event.

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

The param name is event

*
* @since 4.9.0
*
* @param {jQuery.Event} Event.

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

param name is event

if ( 'customize' === pagenow ) {
$( '.customize-themes-notifications' ).append( $adminNotice );
} else {
$( '.wrap' ).find( '> h1' ).after( $adminNotice );

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Sep 29, 2017

may be .children( 'h1' ) would be more expressive to read.

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 29, 2017

Author Collaborator

This is existing code already in trunk.

@westonruter

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 29, 2017

Committed to trunk in r41648.

@westonruter westonruter deleted the trac-37661-redux branch Sep 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.