Chrome: Support custom taxonomies #2340

youknowriad · 2017-08-10T13:22:36Z

This PR introduces support for custom taxonomies. It transforms the current CategorySelector and TagSelector to more generic components: HierarchicalTaxonomy and FlatTaxonomy.

This PR uses the same technique used for Custom Post Types to retrieve the Model and the Collection of a Custom Hierarchy in the API client.

Caveats

This works only for taxonomies with show_in_rest => true (issue being worked-on on Trac)

Testing instructions

Check that the tags/categories selectors works same as master for posts
Add some custom taxonomies like this (you can add it to the functions.php of your theme for example):

register_taxonomy(
   	'genre',
   	'post',
   	array(
   		'label' => __( 'Genre' ),
   		'rewrite' => array( 'slug' => 'genre' ),
   		'hierarchical' => false,
   		'show_ui' => true, 
   		'show_in_rest' => true,
   	)
   );

And check that you can set terms to the post for this custom taxonomy. (try switching the hierarchical flag)

codecov · 2017-08-10T13:40:50Z

Codecov Report

Merging #2340 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2340      +/-   ##
==========================================
+ Coverage   24.69%   24.69%   +<.01%     
==========================================
  Files         152      152              
  Lines        4738     4993     +255     
  Branches      799      853      +54     
==========================================
+ Hits         1170     1233      +63     
- Misses       3014     3158     +144     
- Partials      554      602      +48

Impacted Files	Coverage Δ
...ebar/post-taxonomies/hierarchical-term-selector.js	`0% <0%> (ø)`
...itor/sidebar/post-taxonomies/flat-term-selector.js	`0% <0%> (ø)`
editor/sidebar/post-taxonomies/index.js	`0% <0%> (ø)`	⬆️
blocks/library/separator/index.js	`42.85% <0%> (-7.15%)`	⬇️
components/higher-order/with-focus-return/index.js	`92.85% <0%> (-7.15%)`	⬇️
blocks/library/cover-text/index.js	`29.41% <0%> (-5.59%)`	⬇️
blocks/library/heading/index.js	`18.91% <0%> (-4.9%)`	⬇️
blocks/library/latest-posts/index.js	`7.22% <0%> (-2.78%)`	⬇️
blocks/library/gallery/index.js	`23.8% <0%> (-1.2%)`	⬇️
blocks/library/list/index.js	`6.72% <0%> (-0.51%)`	⬇️
... and 5 more

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 147644b...3bcb09e. Read the comment docs.

jasmussen · 2017-08-10T14:42:13Z

What's the best way to test this?

youknowriad · 2017-08-10T14:47:23Z

@jasmussen good question, I added some instructions to the description

jasmussen · 2017-08-10T14:56:00Z

Yup! Seems to be working for me, nice work! 👍 👍

aduth · 2017-08-10T16:25:07Z

It transforms the current CategorySelector and TagSelector to more generic components: HierarchicalTaxonomy and FlatTaxonomy.

Not sure about the rename, since without context, it's not clear what a "HierarchicalTaxonomy" component is. Might suggest instead: HierarchicalTermSelector, FlatTermSelector.

On the fence about even dropping "Flat" in favor of TermSelector (non-hierarchical) and HierarchicalTermSelector.

aduth · 2017-08-10T16:28:18Z

Do we still want this to show an "Add New Tag" placeholder for the tag type specifically?

(Maybe as a small wrapper of flat term selector)

aduth · 2017-08-10T16:30:45Z

Do custom taxonomies belong under the "Categories & Tags" heading? In Calypso we have Categories & Tags containing.... categories and tags. But custom taxonomies under their own panels.

aduth · 2017-08-10T16:21:54Z

editor/sidebar/post-taxonomies/flat-taxonomy.js

+ * External dependencies
+ */
+import { connect } from 'react-redux';
+import { unescape, find, throttle } from 'lodash';


Note entirely in scope but: The reason I'd renamed this in the other component is because there is a global function by the same name:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/unescape

This is problematic if the import is ever dropped for any reason, since linting will not surface any issues:

gutenberg (add/custom-taxonomies) $ git diff diff --git a/editor/sidebar/post-taxonomies/flat-taxonomy.js b/editor/sidebar/post-taxonomies/flat-taxonomy.js index 995aa17a..d21a7be6 100644 --- a/editor/sidebar/post-taxonomies/flat-taxonomy.js +++ b/editor/sidebar/post-taxonomies/flat-taxonomy.js @@ -2,7 +2,7 @@ * External dependencies */ import { connect } from 'react-redux'; -import { unescape, find, throttle } from 'lodash'; +import { find, throttle } from 'lodash'; /** * WordPress dependencies gutenberg (add/custom-taxonomies) $ npx eslint editor/sidebar/post-taxonomies/flat-taxonomy.js gutenberg (add/custom-taxonomies) $

aduth · 2017-08-10T16:33:23Z

editor/sidebar/post-taxonomies/index.js

+					const TaxonomyComponent = taxonomy.hierarchical ? HierarchicalTaxonomy : FlatTaxonomy;
+					return (
+						<TaxonomyComponent
+							key={ taxonomy.name }


Since name is human-readable label and not necessarily unique, should we use taxonomy.slug instead?

aduth · 2017-08-10T16:34:55Z

lib/client-assets.php

@@ -375,6 +385,18 @@ function gutenberg_extend_wp_api_backbone_client() {
 				return model.prototype.route && route === model.prototype.route.index;
 			} ) );
 		};
+		wp.api.getTaxonomyModel = function( taxonomy ) {


Eugh, why is rest_base a thing 😞

aduth · 2017-08-10T16:36:15Z

lib/client-assets.php

@@ -375,6 +385,18 @@ function gutenberg_extend_wp_api_backbone_client() {
 				return model.prototype.route && route === model.prototype.route.index;
 			} ) );
 		};
+		wp.api.getTaxonomyModel = function( taxonomy ) {
+			var route = '/' + wpApiSettings.versionString + this.taxonomyRestBaseMapping[ taxonomy ] + '/(?P<id>[\\\\d]+)';
+			return _.first( _.filter( wp.api.models, function( model ) {


I know this is copy-paste, but we should really use _.find here so we're not filtering the entire list if match is one of the first entries.

youknowriad · 2017-08-10T16:38:43Z

Do we still want this to show an "Add New Tag" placeholder for the tag type specifically?

I don't like special cases but if we want this, ok.

Do custom taxonomies belong under the "Categories & Tags" heading?

No personal preference for me. We can use a generic "Taxonomies" Panel, or maybe have a Panel per taxonomy to avoid special-casing

youknowriad · 2017-08-10T16:46:23Z

On the fence about even dropping "Flat" in favor of TermSelector (non-hierarchical) and HierarchicalTermSelector.

I prefer leaving "Flat" because it indicates that the component is not usable for hierarchical taxonomies (avoids ambiguity)

aduth · 2017-08-10T18:57:59Z

I have a feeling of déjà vu with this conversation around special treatment of Categories & Tags. I wasn't able to find much prior conversation aside from some of the related effort in Calypso (Automattic/wp-calypso#6744). I seem to recall some conversations with @mtias at the time about how we should treat Categories & Tags as special, independent from other taxonomies (together in their own accordion).

Perhaps good for UX because in most real scenarios they're the common taxonomies?

But also not consistent, in which case we could opt for:

One accordion per taxonomy
All term selectors under a single accordion

Testing this makes me wish we support remembering toggle states of accordions 😆

youknowriad · 2017-08-11T12:14:56Z

But also not consistent, in which case we could opt for:

One accordion per taxonomy
All term selectors under a single accordion

Any thoughts on the which option to choose here? cc @jasmussen @mtias

jasmussen · 2017-08-11T12:29:32Z

Any thoughts on the which option to choose here?

No strong opinion either way, though slightly leaning towards a single accordion. But I don't think this should hold off the PR from getting merged.

Testing this makes me wish we support remembering toggle states of accordions 😆

We should, long term, though.

youknowriad · 2017-08-11T12:34:36Z

Ok I'm merging the PR as is now to include it in today's release

Update podfile.

youknowriad added the General Interface Parts of the UI which don't fall neatly under other labels. label Aug 10, 2017

youknowriad self-assigned this Aug 10, 2017

youknowriad requested review from jasmussen and aduth August 10, 2017 13:22

youknowriad changed the title ~~Chrome: Allow settings custom taxonomies~~ Chrome: Support custom taxonomies Aug 10, 2017

Chrome: Allow settings custom taxonomies

e713c0e

youknowriad force-pushed the add/custom-taxonomies branch from 56f4b62 to e713c0e Compare August 10, 2017 13:40

aduth reviewed Aug 10, 2017

View reviewed changes

youknowriad added 2 commits August 10, 2017 17:43

Taxonomies: Rename lodash's unescape to avoid conflicts

57bea6d

Taxonomies: Rename TermSelectors components

902463b

youknowriad added 2 commits August 10, 2017 17:47

Performance: Replace _.find( _.filter ) with _.find()

8758374

Taxonomies: Use the slug as react key

3bcb09e

youknowriad merged commit 8a77b33 into master Aug 11, 2017

youknowriad deleted the add/custom-taxonomies branch August 11, 2017 12:34

aduth mentioned this pull request Nov 7, 2017

Custom Taxonomies Don't Show in the sidebar #3369

Closed

ceyhun pushed a commit that referenced this pull request Jun 17, 2020

Merge pull request #2340 from wordpress-mobile/issue/update_podfile_lock

2522c2b

Update podfile.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chrome: Support custom taxonomies #2340

Chrome: Support custom taxonomies #2340

youknowriad commented Aug 10, 2017 •

edited

Loading

codecov bot commented Aug 10, 2017 •

edited

Loading

jasmussen commented Aug 10, 2017

youknowriad commented Aug 10, 2017

jasmussen commented Aug 10, 2017

aduth commented Aug 10, 2017

aduth commented Aug 10, 2017

aduth commented Aug 10, 2017

aduth Aug 10, 2017

aduth Aug 10, 2017

aduth Aug 10, 2017

aduth Aug 10, 2017

youknowriad commented Aug 10, 2017

youknowriad commented Aug 10, 2017

aduth commented Aug 10, 2017

youknowriad commented Aug 11, 2017

jasmussen commented Aug 11, 2017

youknowriad commented Aug 11, 2017

Chrome: Support custom taxonomies #2340

Chrome: Support custom taxonomies #2340

Conversation

youknowriad commented Aug 10, 2017 • edited Loading

codecov bot commented Aug 10, 2017 • edited Loading

Codecov Report

jasmussen commented Aug 10, 2017

youknowriad commented Aug 10, 2017

jasmussen commented Aug 10, 2017

aduth commented Aug 10, 2017

aduth commented Aug 10, 2017

aduth commented Aug 10, 2017

aduth Aug 10, 2017

Choose a reason for hiding this comment

aduth Aug 10, 2017

Choose a reason for hiding this comment

aduth Aug 10, 2017

Choose a reason for hiding this comment

aduth Aug 10, 2017

Choose a reason for hiding this comment

youknowriad commented Aug 10, 2017

youknowriad commented Aug 10, 2017

aduth commented Aug 10, 2017

youknowriad commented Aug 11, 2017

jasmussen commented Aug 11, 2017

youknowriad commented Aug 11, 2017

youknowriad commented Aug 10, 2017 •

edited

Loading

codecov bot commented Aug 10, 2017 •

edited

Loading