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

Add new Categories block #2102

Merged
merged 16 commits into from Aug 7, 2017

Conversation

@tyxla
Copy link
Member

tyxla commented Jul 31, 2017

Purpose

This PR introduces a new Categories dynamic block, fixes #1790.

Features

The new block comes with the following alignment options: none, left, right, center, full. It also supports the following attributes, which are similar to the core Categories widget:

  • Display as dropdown - default is false, which displays an unordered list.
  • Show post counts - default is false, indicates these are totally hidden.
  • Show hierarchy - default is false, meaning a flat, non-hierarchical list.

The server-side rendering part takes advantage of what the core widget uses - wp_list_categories() for the unordered list, and wp_dropdown_categories for the dropdown.

Preview

List view, no post counts, no hierarchy

List view, with post counts, no hierarchy

List view, no post counts, with hierarchy

List view, with post counts, with hierarchy

Dropdown view, without post counts, without hierarchy

Dropdown view, with post counts, without hierarchy

Dropdown view, without post counts, with hierarchy

Dropdown view, with post counts, with hierarchy

Block control options

Questions

The script for redirecting users to a category once they change it in the dropdown field in the frontend - I've borrowed it from the Categories core widget. I felt it would make sense to be consistent with what we have in the core, but if you folks have better suggestions, I'm all ears! Thanks!

@tyxla tyxla self-assigned this Jul 31, 2017

@tyxla tyxla force-pushed the try/categories-block branch from fdbdbd5 to f694b1f Jul 31, 2017

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 31, 2017

Codecov Report

Merging #2102 into master will decrease coverage by 0.28%.
The diff coverage is 3.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2102      +/-   ##
==========================================
- Coverage    22.3%   22.01%   -0.29%     
==========================================
  Files         137      139       +2     
  Lines        4291     4356      +65     
  Branches      722      730       +8     
==========================================
+ Hits          957      959       +2     
- Misses       2815     2870      +55     
- Partials      519      527       +8
Impacted Files Coverage Δ
blocks/library/categories/data.js 0% <0%> (ø)
blocks/library/categories/index.js 3.22% <3.22%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 207274b...c501a4b. Read the comment docs.

@tyxla tyxla force-pushed the try/categories-block branch 2 times, most recently from f604a79 to 380769b Jul 31, 2017

@tyxla tyxla referenced this pull request Jul 31, 2017

Closed

Add Block: Categories #1790

@tyxla tyxla requested review from youknowriad , mtias , aduth and iseulde Jul 31, 2017

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jul 31, 2017

This works great! Thanks so much for working on it.

This PR feels very very ready to go.

It doesn't show empty categories by default, it seems. Is the same behavior as the widget?

@@ -0,0 +1,13 @@
/**
* Returns a Promise with the categories or an error on failure.

This comment has been minimized.

@aduth

aduth Jul 31, 2017

Member

Technically it returns a jqXHR which implements the jQuery.Deffered interface.

http://backbonejs.org/#Collection-fetch

This comment has been minimized.

@tyxla

tyxla Aug 3, 2017

Author Member

👍 86afe7a

];
}

componentWillUnmount() {

This comment has been minimized.

@aduth

aduth Jul 31, 2017

Member

Not prescribed, but general convention that render should be last method defined in a component class. I tend to prefer to keep lifecycle methods together toward the top of the class.

What do you think? Maybe we should document this, since I find I'm recommending it fairly often.

This comment has been minimized.

@tyxla

tyxla Aug 3, 2017

Author Member

I agree, updated in 2af85c8.

}

toggleDisplayAsDropdown() {
const { displayAsDropdown } = this.props.attributes;

This comment has been minimized.

@aduth

aduth Jul 31, 2017

Member

Minor (Stylistic): We could avoid nested property access by including attributes in the destructure of this.props:

const { attributes, setAttributes } = this.props;
const { displayAsDropdown } = attributes;

This comment has been minimized.

@tyxla

tyxla Aug 3, 2017

Author Member

👍 30ba95e

this.categoriesRequest = getCategories();

this.categoriesRequest
.then( categories => this.setState( { categories } ) );

This comment has been minimized.

@aduth

aduth Jul 31, 2017

Member

Can we confirm whether this callback will be invoked if the request is aborted while in-flight? At that point the component would no longer be mounted either, so setState could cause trouble.

This comment has been minimized.

@tyxla

tyxla Aug 3, 2017

Author Member

Sure - it will not be invoked, as the deferred .then is executed only if the request is resolved.

getCategories( parentId = null ) {
const { categories } = this.state;
if ( ! categories.length ) {
return [];

This comment has been minimized.

@aduth

aduth Jul 31, 2017

Member

Should we return categories; here? Why do we need this condition anyways? I doubt Array#filter on an empty array has much overhead to be worried about.

This comment has been minimized.

@tyxla

tyxla Aug 3, 2017

Author Member

Yeah, good call about returning categories here - updated in 0fa3f29. I'm more inclined to keep the condition though - no need to continue if categories are not loaded yet.


return (
<li key={ category.id }>
<a href={ category.link } target="_blank">{ category.name.trim() || __( '(Untitled)' ) }</a>

This comment has been minimized.

@aduth

aduth Jul 31, 2017

Member

Curious since I've dealt with issues before: How well does this handle entities, like an ampersand & ? Might need to do some unescaping.

This comment has been minimized.

@tyxla

tyxla Aug 3, 2017

Author Member

👍 Good call, done in 428d791.


return [
<option key={ category.id }>
{ new Array( level * 3 ).fill( '\xa0' ) }

This comment has been minimized.

@aduth

aduth Jul 31, 2017

Member

We don't include a polyfill (yet). This will error in IE11:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/fill#Browser_compatibility

Try instead:

Array.apply( null, new Array( level * 3 ) ).map( () => '\xa0' );

Or with Lodash:

_.times( 5, () => '\xa0' );

https://lodash.com/docs/4.17.4#times

This comment has been minimized.

@tyxla

tyxla Aug 3, 2017

Author Member

Right, updated in aa5d4ad.

{ category.name.trim() || __( '(Untitled)' ) }
{
!! showPostCounts
? ` ( ${ category.count } )`

This comment has been minimized.

@aduth

aduth Jul 31, 2017

Member

Above we don't include spaces in parentheses around category.count, but here we do. Should we be consistent?

This comment has been minimized.

@tyxla

tyxla Aug 3, 2017

Author Member

Of course! Updated in f1f7f0b.

);
if ( ! empty( $attributes['displayAsDropdown'] ) ) {
$id = 'wp-block-categories-' . wp_rand();

This comment has been minimized.

@aduth

aduth Jul 31, 2017

Member

Why wp_rand ? Uniqueness? We're not guaranteed unique values between two subsequent wp_rand calls.

This comment has been minimized.

@tyxla

tyxla Aug 3, 2017

Author Member

Makes sense. I've updated it to use an increasing static counter, see c501a4b

location.href = "<?php echo home_url(); ?>/?cat=" + dropdown.options[ dropdown.selectedIndex ].value;
}
}
dropdown.onchange = onCatChange;

This comment has been minimized.

@aduth

aduth Jul 31, 2017

Member

Should this apply here: https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/no-onchange.md ?

I could see it being frustrating to use by keyboard if it navigated by keying through the list of options.

This comment has been minimized.

@tyxla

tyxla Aug 3, 2017

Author Member

I'm more inclined to keep the behavior we have for the core widget here - I like keeping things consistent. If we update the core widget to use onBlur, I'd be happy to update this one as well.

export function getCategories() {
const categoriesCollection = new wp.api.collections.Categories();

const categories = categoriesCollection.fetch();

This comment has been minimized.

@westonruter

westonruter Jul 31, 2017

Member

Will this not introduce the possibility of there being many requests when there is a large deeply-nested category tree?

This comment has been minimized.

@tyxla

tyxla Aug 3, 2017

Author Member

Not really - this one will be called only per a Categories block.

This comment has been minimized.

@westonruter

westonruter Aug 3, 2017

Member

I see. I was confusing this getCategories function with the getCategories method on the component, and the latter is called recursively.

@@ -27,4 +27,5 @@
// Register server-side code for individual blocks.
require_once dirname( __FILE__ ) . '/lib/blocks/latest-posts.php';
require_once dirname( __FILE__ ) . '/lib/blocks/categories.php';

This comment has been minimized.

@westonruter

westonruter Jul 31, 2017

Member

This needs to be changed in light of #2014.

This comment has been minimized.

@tyxla

tyxla Aug 3, 2017

Author Member

This was fixed with a combination of rebase and f23a5da.

/**
* WordPress dependencies
*/
import { Component } from 'element';

This comment has been minimized.

@aduth

aduth Aug 3, 2017

Member

Note with #2172, these need to be updated to prefix the dependencies with @wordpress/. You will need to perform a rebase against the latest version of master and apply your changes:

git fetch origin
git rebase origin/master

This comment has been minimized.

@tyxla

tyxla Aug 3, 2017

Author Member

👍 Rebased and updated in 515dad4.

@tyxla tyxla force-pushed the try/categories-block branch from f63ded5 to 2cc2804 Aug 3, 2017

@tyxla

This comment has been minimized.

Copy link
Member Author

tyxla commented Aug 3, 2017

@aduth @westonruter thanks so much for the constructive feedback!

I've addressed it all, feel free to have a new look.

@mtias

mtias approved these changes Aug 3, 2017

Copy link
Contributor

mtias left a comment

This looks good to me for a first iteration on categories.

@tyxla

This comment has been minimized.

Copy link
Member Author

tyxla commented Aug 4, 2017

I'm happy to give this one a final round of testing and merge it, if everyone agrees it's good.

@mtias mtias merged commit e5130c3 into master Aug 7, 2017

3 checks passed

codecov/project 22.01% (-0.29%) compared to cf0d37c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mtias mtias deleted the try/categories-block branch Aug 7, 2017

@gziolo gziolo moved this from In Development to Done in Dynamic Blocks: Widgets, Shortcodes, and Menus Nov 3, 2017

@afercia afercia added the Widgets label Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment