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

<CategorySelect> support post_tags and hierarchical = false taxonomy. #13076

Merged
merged 5 commits into from
Jan 22, 2019
Merged

<CategorySelect> support post_tags and hierarchical = false taxonomy. #13076

merged 5 commits into from
Jan 22, 2019

Conversation

torounit
Copy link
Member

Description

<CategorySelect> support custom taxonomy with hierarchical = true now. but not working
hierarchical = false. fixed it.

How has this been tested?

Add this code. and check core/latest-posts block.

add_filter( 'register_taxonomy_args', function ( $args, $taxonomy ) {
	if ( $taxonomy === 'category' ) {
		$args['hierarchical'] = false;
	}
	return $args;
}, 10, 2 );

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@torounit torounit changed the title CategorySelect support post_tags and hierarchical = false taxonomy. <CategorySelect> support post_tags and hierarchical = false taxonomy. Dec 22, 2018
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @torounit, thank you for your contribution 👍
Unfortunately, to not have cyclic dependencies the buildTermsTree logic is duplicated
https://github.com/WordPress/gutenberg/blob/feature/buildTermsTree-support-tags/packages/editor/src/utils/terms.js#L14.
Would it be possible to update the other version?

@torounit
Copy link
Member Author

@jorgefilipecosta Thanks reviewing!

found in same method in packages/editor/src/utils/terms.js and updated.

@torounit
Copy link
Member Author

@jorgefilipecosta need update test ?

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorgefilipecosta need update test ?

Yes we need to update the test case on lines https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/utils/test/terms.js#L7-L11

It would also be good if we could add some additional tests specific to this logic.

const termsByParent = groupBy( flatTerms, 'parent' );
const flatTermsWithParent = flatTerms.map( ( term ) => {
return {
parent: 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this fix also work if the parent was null instead of 0? This makes it clear that no parent exists in this case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed to use parent: null

@torounit
Copy link
Member Author

torounit commented Jan 8, 2019

@jorgefilipecosta use null instead 0. and updated tests.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your interactions @torounit. I think the PR is ready 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Latest Comments Affects the Latest Comments Block [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants