Made each taxonomy appear on its own panel. #5183

jorgefilipecosta · 2018-02-21T18:50:04Z

This PR promotes each taxonomy to be a top-level panel (equal to the classic editor).

Closes: #4599

How Has This Been Tested?

Open post editor, verify now we don't have panel named "Categories & Tags" but a panel for categories and another one for tabs. Open and close the panels and verify things work as expected.
Change the categories & tags and check the functionality was not affected.
Add a custom taxonomy e.g: Type of Project, verify the taxonomy is rendered with the correct labels and works as expected.

Screenshots (jpeg or gifs if applicable):

aduth

Seeing key warning:

react.0406e355.js:336 Warning: Each child in an array or iterator should have a unique "key" prop.

Check the render method of PostTaxonomies. See https://fb.me/react-warning-keys for more information.

aduth · 2018-02-23T05:18:16Z

edit-post/components/sidebar/post-taxonomies/taxonomy-panel.js

+function TaxonomyPanel( { taxonomy, isOpened, onTogglePanel, panelName, children } ) {
+	const taxonomyMenuName = get( taxonomy, [ 'labels', 'menu_name' ] );
+	if ( ! taxonomyMenuName ) {
+		return;


A component cannot return undefined from its render. It should return null.

aduth · 2018-02-23T05:18:35Z

edit-post/components/sidebar/post-taxonomies/taxonomy-panel.js

+	}
+	return (
+		<PanelBody
+			key={ panelName }


I don't think key is effective in this context?

aduth · 2018-02-23T05:21:51Z

edit-post/components/sidebar/post-taxonomies/taxonomy-panel.js

+			return toggleGeneralSidebarEditorPanel( panelName );
+		},
+	},
+	( stateProps, dispatchProps, ownProps ) => {


See #5090 and #5098 (comment) for more on my distaste for the mergeProps function. Specifically that returning a function here will cause the component to always re-render. Via withDispatch of #5137, we can probably avoid this now.

aduth · 2018-02-23T05:22:14Z

edit-post/components/sidebar/post-taxonomies/taxonomy-panel.js

+		};
+	},
+	{
+		onTogglePanel( panelName ) {


Minor: This could be simplified to:

{ onTogglePanel: toggleGeneralSidebarEditorPanel, },

aduth · 2018-02-23T05:24:20Z

editor/components/post-taxonomies/flat-term-selector.js

@@ -171,7 +171,6 @@ class FlatTermSelector extends Component {

 		return (
 			<div className="editor-post-taxonomies__flat-terms-selector">


Do we need the wrapper anymore?

I see there's a lingering style for it, but I'm wondering if we need that either:

https://github.com/WordPress/gutenberg/blob/adb821746d9803ae304c57a750b824e08cdcacef/editor/components/post-taxonomies/style.scss#L1-L4

Why doesn't .components-panel__body-title add its own bottom margin rather than rely on children to add this themselves?

I totally agree with your comment .components-panel__body-title should add its own margin. I wanted to avoid that because it would be a change that affected all the application. But I checked that in inspector controls there was a rule that added this margin. So I removed the rule from there and added it to the component. The changes should be unnoticeable but allowed to remove the wrappers as suggested 👍

aduth · 2018-02-23T05:25:57Z

editor/components/post-taxonomies/index.js

+		);
+	} );
+
+	// Fragment is used because enzyme shallow does not seems to handle correctly components returning just arrays.


Are you sure it's not just that shallow won't traverse into the children ? Have you tried with mount instead?

Not sure of the reason but all my tries to test the component returning an array failed and I observed that the debug method returned totally invalid data. At the time I found a recent PR on enzyme repository, related to this enzymejs/enzyme#1498, maybe these changes are not yet available in our version.

I also tried to use mount instead, but unfortunately, it was not possible because of errors related with our withAPIData HOC I think we would need to mock it to use mount here, which was something unnecessary for testing purposes of this component.

It's very unintuitive, but this seemed to work okay for me:

diff --git a/editor/components/post-taxonomies/index.js b/editor/components/post-taxonomies/index.js index d7f28db94..200d61f06 100644 --- a/editor/components/post-taxonomies/index.js +++ b/editor/components/post-taxonomies/index.js @@ -8,7 +8,7 @@ import { filter, identity, includes } from 'lodash'; * WordPress dependencies */ import { withAPIData } from '@wordpress/components'; -import { compose, Fragment } from '@wordpress/element'; +import { compose } from '@wordpress/element'; /** * Internal dependencies @@ -20,7 +20,8 @@ import { getCurrentPostType } from '../../store/selectors'; export function PostTaxonomies( { postType, taxonomies, taxonomyWrapper = identity } ) { const availableTaxonomies = filter( taxonomies.data, ( taxonomy ) => includes( taxonomy.types, postType ) ); - const taxonomyComponents = availableTaxonomies.map( ( taxonomy ) => { + + return availableTaxonomies.map( ( taxonomy ) => { const TaxonomyComponent = taxonomy.hierarchical ? HierarchicalTermSelector : FlatTermSelector; return taxonomyWrapper( <TaxonomyComponent @@ -31,14 +32,6 @@ export function PostTaxonomies( { postType, taxonomies, taxonomyWrapper = identi taxonomy ); } ); - - // Fragment is used because enzyme shallow does not seems to handle correctly components returning just arrays. - // Using debug method it was possible to verify shallow always output <undefined /> when arrays are used. - return ( - <Fragment> - { taxonomyComponents } - </Fragment> - ); } const applyConnect = connect( diff --git a/editor/components/post-taxonomies/test/index.js b/editor/components/post-taxonomies/test/index.js index 42cfed70f..1f21845a1 100644 --- a/editor/components/post-taxonomies/test/index.js +++ b/editor/components/post-taxonomies/test/index.js @@ -16,7 +16,7 @@ describe( 'PostTaxonomies', () => { <PostTaxonomies postType="page" taxonomies={ taxonomies } /> ); - expect( wrapper.children() ).toHaveLength( 0 ); + expect( wrapper.at( 0 ) ).toHaveLength( 0 ); } ); it( 'should render taxonomy components for taxonomies assigned to post type', () => { @@ -43,8 +43,8 @@ describe( 'PostTaxonomies', () => { <PostTaxonomies postType="page" taxonomies={ taxonomies } /> ); - expect( wrapper.children() ).toHaveLength( 1 ); - expect( wrapper.childAt( 0 ).props() ).toEqual( { + expect( wrapper.at( 0 ) ).toHaveLength( 1 ); + expect( wrapper.at( 0 ).at( 0 ).props() ).toEqual( { slug: 'category', restBase: 'categories', } );

enzymejs/enzyme#1149 is the (open) parent issue for array-returning components in Enzyme.

aduth · 2018-02-28T22:39:32Z

edit-post/components/sidebar/post-taxonomies/taxonomy-panel.js

+	withDispatch( ( dispatch, ownProps ) => ( {
+		onTogglePanel: () => {
+			if ( ownProps.panelName ) {
+				return dispatch( 'core/edit-post' ).


This doesn't need to return a value.

aduth · 2018-02-28T22:40:19Z

edit-post/components/sidebar/post-taxonomies/taxonomy-panel.js

+		onTogglePanel: () => {
+			if ( ownProps.panelName ) {
+				return dispatch( 'core/edit-post' ).
+					toggleGeneralSidebarEditorPanel( ownProps.panelName );


Minor: If we're concerned about line length, we could invert the condition to an early return to avoid the tab.

if ( ! ownProps.panelName ) { return; } dispatch( 'core/edit-post' ).toggleGeneralSidebarEditorPanel( ownProps.panelName );

aduth · 2018-02-28T22:42:09Z

edit-post/components/sidebar/post-taxonomies/taxonomy-panel.js

+	} ),
+	withDispatch( ( dispatch, ownProps ) => ( {
+		onTogglePanel: () => {
+			if ( ownProps.panelName ) {


Under what real-world conditions will panelName be falsey? And in those cases, is it acceptable that toggling the panel then does nothing?

I expected when loading, but on a second thought when loading the array is empty and we don't render taxonomy-panel at all, so this logic was not needed.

aduth · 2018-02-28T23:03:19Z

editor/components/post-taxonomies/index.js

+		);
+	} );
+
+	// Fragment is used because enzyme shallow does not seems to handle correctly components returning just arrays.


It's very unintuitive, but this seemed to work okay for me:

diff --git a/editor/components/post-taxonomies/index.js b/editor/components/post-taxonomies/index.js index d7f28db94..200d61f06 100644 --- a/editor/components/post-taxonomies/index.js +++ b/editor/components/post-taxonomies/index.js @@ -8,7 +8,7 @@ import { filter, identity, includes } from 'lodash'; * WordPress dependencies */ import { withAPIData } from '@wordpress/components'; -import { compose, Fragment } from '@wordpress/element'; +import { compose } from '@wordpress/element'; /** * Internal dependencies @@ -20,7 +20,8 @@ import { getCurrentPostType } from '../../store/selectors'; export function PostTaxonomies( { postType, taxonomies, taxonomyWrapper = identity } ) { const availableTaxonomies = filter( taxonomies.data, ( taxonomy ) => includes( taxonomy.types, postType ) ); - const taxonomyComponents = availableTaxonomies.map( ( taxonomy ) => { + + return availableTaxonomies.map( ( taxonomy ) => { const TaxonomyComponent = taxonomy.hierarchical ? HierarchicalTermSelector : FlatTermSelector; return taxonomyWrapper( <TaxonomyComponent @@ -31,14 +32,6 @@ export function PostTaxonomies( { postType, taxonomies, taxonomyWrapper = identi taxonomy ); } ); - - // Fragment is used because enzyme shallow does not seems to handle correctly components returning just arrays. - // Using debug method it was possible to verify shallow always output <undefined /> when arrays are used. - return ( - <Fragment> - { taxonomyComponents } - </Fragment> - ); } const applyConnect = connect( diff --git a/editor/components/post-taxonomies/test/index.js b/editor/components/post-taxonomies/test/index.js index 42cfed70f..1f21845a1 100644 --- a/editor/components/post-taxonomies/test/index.js +++ b/editor/components/post-taxonomies/test/index.js @@ -16,7 +16,7 @@ describe( 'PostTaxonomies', () => { <PostTaxonomies postType="page" taxonomies={ taxonomies } /> ); - expect( wrapper.children() ).toHaveLength( 0 ); + expect( wrapper.at( 0 ) ).toHaveLength( 0 ); } ); it( 'should render taxonomy components for taxonomies assigned to post type', () => { @@ -43,8 +43,8 @@ describe( 'PostTaxonomies', () => { <PostTaxonomies postType="page" taxonomies={ taxonomies } /> ); - expect( wrapper.children() ).toHaveLength( 1 ); - expect( wrapper.childAt( 0 ).props() ).toEqual( { + expect( wrapper.at( 0 ) ).toHaveLength( 1 ); + expect( wrapper.at( 0 ).at( 0 ).props() ).toEqual( { slug: 'category', restBase: 'categories', } );

enzymejs/enzyme#1149 is the (open) parent issue for array-returning components in Enzyme.

jorgefilipecosta · 2018-03-01T11:08:43Z

Hi @aduth thank you for providing a possible approach to handle enzyme array limitation 👍, all the feedback was applied.

aduth · 2018-03-01T16:58:45Z

I'd like for this to make it into the 2.3 release, but am also wary about the small design change here in that there's marginally more margin added to the bottom of sidebar panel titles. The removed styles applied only to block inspector panels, not those in the document inspector

Before (Focused)	After (Focused)

Before (Unfocused)	After (Unfocused)

cc @jasmussen

aduth · 2018-03-01T17:07:26Z

editor/components/post-taxonomies/index.js

+		const TaxonomyComponent = taxonomy.hierarchical ? HierarchicalTermSelector : FlatTermSelector;
+		return taxonomyWrapper(
+			<TaxonomyComponent
+				key={ taxonomy.slug }


I think there's some redundancy in key application. Technically this prop is not needed (nor used), but I do think it should be the responsibility of this component, not the callback for taxonomyWrapper, to apply the key. This could be done in one of two ways:

Add a wrapper element to the return value of this map callback with the key

Use cloneElement to assign key as a prop into the return value of taxonomyWrapper

Yes, it makes sense to define the key here, I used a wrapper as it looks like the simplest approach.

aduth · 2018-03-01T17:08:47Z

editor/components/post-taxonomies/hierarchical-term-selector.js

+			...this.renderTerms( availableTermsTree ),
+			! loading && (
+				<button
+					key="term-add-btn"


Minor: Abbreviating seems to serve only to introduce some ambiguity here.

The abbreviation was removed.

aduth · 2018-03-01T17:09:28Z

editor/components/post-taxonomies/hierarchical-term-selector.js

+						required
+					/>
+					{ !! availableTerms.length &&
+					<TreeSelect


Should be indented one more (how it was previously)

jasmussen · 2018-03-01T17:28:57Z

I'd like for this to make it into the 2.3 release, but am also wary about the small design change here in that there's marginally more margin added to the bottom of sidebar panel titles. The removed styles applied only to block inspector panels, not those in the document inspector

Took a look. Here's the whole panel:

Here's the extra margin the change makes:

I think this is fine. 👍 👍 for the change and shipping, we can always revisit later.

Now that you made me look, however, and this is unrelated to this PR, I did notice that there's too much padding in the Status and Comments panels:

This seems to be from the .components-panel__row top-margin not collapsing:

We can either convert that margin to a bottom margin instead, or do something like this:

.components-panel__row:first-of-type {
    margin-top: 0;
}

That would address it:

But this can be done separately.

jorgefilipecosta · 2018-03-02T11:52:13Z

Hi @aduth and @jasmussen thank you for your feedback. I end up adding the CSS rule referred by @jasmussen, I think it improves the design compared to what we had, thank you @jasmussen 👍 I think the rule may be fragile e.g: if panel title was a div it would not work, but right now it seems like our best bet and it solves the problem. Maybe the panel body should have title and content in separate dom containers and the :first-child selector could be used for rows, it is something to check and discuss after, but from the design point of view as things are right now the result would be the same.

The design now looks like this:

jorgefilipecosta · 2018-03-02T14:20:25Z

Test cases were updated, the addition of the wrapper to allow keys to be set in the right place made the props test case even less intuitive, that test case was removed and another test case that tests the rendering of multiple taxonomies was added. I think this made tests cases simpler to understand and still relevant.

jorgefilipecosta · 2018-03-02T14:42:43Z

Hi @karmatosed, can you please have a final look regarding the user experience perspective?

aduth

Code-wise, looks good 👍

aduth · 2018-03-02T17:19:22Z

components/panel/style.scss

@@ -134,6 +134,10 @@
 	&:empty {
 		margin-top: 0;
 	}
+
+	&:first-of-type {
+		margin-top: 0px;


Minor: 0 doesn't need a unit. Further, this could be merged into the selector above:

&:empty, &:first-of-type { margin-top: 0; }

Before all taxonomies appeared under "Categories & Tags" panel.

karmatosed

Looks good to me, thanks for iterating on this.

jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Feature] Document Settings Document settings experience labels Feb 21, 2018

jorgefilipecosta self-assigned this Feb 21, 2018

jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Feb 22, 2018

jorgefilipecosta force-pushed the update/made-each-taxonomy-appear-on-its-own-panel branch 2 times, most recently from 34d4832 to adb8217 Compare February 22, 2018 12:59

jorgefilipecosta removed the [Status] In Progress Tracking issues with work in progress label Feb 22, 2018

aduth reviewed Feb 23, 2018

View reviewed changes

jorgefilipecosta force-pushed the update/made-each-taxonomy-appear-on-its-own-panel branch 3 times, most recently from ecb5243 to db17ed3 Compare February 28, 2018 16:27

aduth reviewed Feb 28, 2018

View reviewed changes

jorgefilipecosta force-pushed the update/made-each-taxonomy-appear-on-its-own-panel branch 3 times, most recently from 436b007 to 8e8e91d Compare March 1, 2018 11:05

aduth added the Needs Design Feedback Needs general design feedback. label Mar 1, 2018

aduth reviewed Mar 1, 2018

View reviewed changes

jorgefilipecosta force-pushed the update/made-each-taxonomy-appear-on-its-own-panel branch from 8e8e91d to 7b357ab Compare March 2, 2018 11:43

aduth mentioned this pull request Mar 2, 2018

Title of panel Categories & Tags is wrong when only one taxonomy is used #4599

Closed

jorgefilipecosta force-pushed the update/made-each-taxonomy-appear-on-its-own-panel branch from 7b357ab to 88cdbf5 Compare March 2, 2018 14:06

jorgefilipecosta requested a review from karmatosed March 2, 2018 14:42

aduth added this to the 2.3 milestone Mar 2, 2018

aduth approved these changes Mar 2, 2018

View reviewed changes

jorgefilipecosta force-pushed the update/made-each-taxonomy-appear-on-its-own-panel branch from 495fb69 to 53d2dd9 Compare March 2, 2018 17:33

Made each taxonomy appear on its own panel.

3e86770

Before all taxonomies appeared under "Categories & Tags" panel.

karmatosed approved these changes Mar 2, 2018

View reviewed changes

jorgefilipecosta force-pushed the update/made-each-taxonomy-appear-on-its-own-panel branch from 53d2dd9 to 3e86770 Compare March 2, 2018 17:35

jorgefilipecosta merged commit e505e02 into master Mar 2, 2018

jorgefilipecosta deleted the update/made-each-taxonomy-appear-on-its-own-panel branch March 2, 2018 17:43

oandregal mentioned this pull request Aug 1, 2018

Remove unused code in taxonomies panel #8349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made each taxonomy appear on its own panel. #5183

Made each taxonomy appear on its own panel. #5183

jorgefilipecosta commented Feb 21, 2018

aduth left a comment

aduth Feb 23, 2018

aduth Feb 23, 2018

aduth Feb 23, 2018

aduth Feb 23, 2018

aduth Feb 23, 2018

jorgefilipecosta Feb 28, 2018

aduth Feb 23, 2018

jorgefilipecosta Feb 28, 2018 •

edited

Loading

aduth Feb 28, 2018

aduth Feb 28, 2018

aduth Feb 28, 2018

aduth Feb 28, 2018

jorgefilipecosta Mar 1, 2018

aduth Feb 28, 2018

jorgefilipecosta commented Mar 1, 2018

aduth commented Mar 1, 2018

aduth Mar 1, 2018

jorgefilipecosta Mar 2, 2018

aduth Mar 1, 2018

jorgefilipecosta Mar 2, 2018

aduth Mar 1, 2018

jorgefilipecosta Mar 2, 2018

jasmussen commented Mar 1, 2018

jorgefilipecosta commented Mar 2, 2018 •

edited

Loading

jorgefilipecosta commented Mar 2, 2018 •

edited

Loading

jorgefilipecosta commented Mar 2, 2018

aduth left a comment

aduth Mar 2, 2018

karmatosed left a comment

		@@ -171,7 +171,6 @@ class FlatTermSelector extends Component {

		return (
		<div className="editor-post-taxonomies__flat-terms-selector">

Made each taxonomy appear on its own panel. #5183

Made each taxonomy appear on its own panel. #5183

Conversation

jorgefilipecosta commented Feb 21, 2018

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Mar 1, 2018

aduth commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Mar 1, 2018

jorgefilipecosta commented Mar 2, 2018 • edited Loading

jorgefilipecosta commented Mar 2, 2018 • edited Loading

jorgefilipecosta commented Mar 2, 2018

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karmatosed left a comment

Choose a reason for hiding this comment

jorgefilipecosta Feb 28, 2018 •

edited

Loading

jorgefilipecosta commented Mar 2, 2018 •

edited

Loading

jorgefilipecosta commented Mar 2, 2018 •

edited

Loading