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

Chrome: Hide The Taxonomies Panel if no taxonomy is available for the current CPT #4166

Merged
merged 1 commit into from Dec 27, 2017

Conversation

Projects
None yet
2 participants
@youknowriad
Contributor

youknowriad commented Dec 26, 2017

closes #3943

This PR hides the taxonomies panel if the CPT doesn't support any taxonomy.

Testing instructions

  • Create a CPT without any assigned taxonomy
  • Create a post with this CPT.
  • The taxonomies panel should not show up.

@youknowriad youknowriad self-assigned this Dec 26, 2017

);
const applyWithAPIData = withAPIData( () => ( {
taxonomies: '/wp/v2/taxonomies?context=edit',

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Dec 26, 2017

Member

In file /editor/components/post-taxonomies/index.js with are repeating the applyConnect and applyWithAPIData logic. An option to avoid this repetition would be moving the grab taxonomies logic (applyConnect and applyWithAPIData) to file sidebar/post-taxonomies/index.js and pass the available taxonomies to PostTaxonomiesForm and to PostTaxonomiesCheck (or do the check directly in sidebar/post-taxonomies/index.js). Regarding the checking instead of filter + checking array length 0, we can use "some" so we avoid iterating over all the array and building a new filtered array.

@jorgefilipecosta

jorgefilipecosta Dec 26, 2017

Member

In file /editor/components/post-taxonomies/index.js with are repeating the applyConnect and applyWithAPIData logic. An option to avoid this repetition would be moving the grab taxonomies logic (applyConnect and applyWithAPIData) to file sidebar/post-taxonomies/index.js and pass the available taxonomies to PostTaxonomiesForm and to PostTaxonomiesCheck (or do the check directly in sidebar/post-taxonomies/index.js). Regarding the checking instead of filter + checking array length 0, we can use "some" so we avoid iterating over all the array and building a new filtered array.

This comment has been minimized.

@youknowriad

youknowriad Dec 27, 2017

Contributor

Funny enough, I commented on your PR before replying here :) #4175

So, the idea is that we do not guarantee that someone using the PostTaxonomies is wrapping it with the Check since it's a reusable component. So it's fine to duplicate to ensure components portability.

@youknowriad

youknowriad Dec 27, 2017

Contributor

Funny enough, I commented on your PR before replying here :) #4175

So, the idea is that we do not guarantee that someone using the PostTaxonomies is wrapping it with the Check since it's a reusable component. So it's fine to duplicate to ensure components portability.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Dec 27, 2017

Member

Hi @youknowriad, I added this comment before checking your review :) I understood the reasons of the duplication, so the only minor change I would do before the merge would be changing PostTaxonomiesCheck to something like const hasTaxonomies = some( taxonomies.data, ( taxonomy ) => includes( taxonomy.types, postType ) ); if( hasTaxonomies ).... But the performance difference should not be noticeable in this case so feel free to ignore this nitpick.

@jorgefilipecosta

jorgefilipecosta Dec 27, 2017

Member

Hi @youknowriad, I added this comment before checking your review :) I understood the reasons of the duplication, so the only minor change I would do before the merge would be changing PostTaxonomiesCheck to something like const hasTaxonomies = some( taxonomies.data, ( taxonomy ) => includes( taxonomy.types, postType ) ); if( hasTaxonomies ).... But the performance difference should not be noticeable in this case so feel free to ignore this nitpick.

This comment has been minimized.

@youknowriad

youknowriad Dec 27, 2017

Contributor

I like this suggestion, updating.

@youknowriad

youknowriad Dec 27, 2017

Contributor

I like this suggestion, updating.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 27, 2017

Contributor

Thanks for the review @jorgefilipecosta

Contributor

youknowriad commented Dec 27, 2017

Thanks for the review @jorgefilipecosta

youknowriad referenced this pull request Dec 27, 2017

@youknowriad youknowriad merged commit 9646747 into master Dec 27, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/taxonomies-panel branch Dec 27, 2017

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