-
Notifications
You must be signed in to change notification settings - Fork 2k
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
CPT / Editor: Enable custom taxonomies for posts and pages #6744
Conversation
if ( config.isEnabled( 'manage/custom-post-types' ) ) { | ||
taxonomies = ( | ||
<EditorDrawerTaxonomies | ||
key="taxonomies" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary because createFragment
assigns a key itself, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary because
createFragment
assigns a key itself, right?
Yes, I believe you're correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key
removed in rebased 6faa308
Looks good apart from the notes above, and works well in my testing on my primary site: Partially addresses #720 (for any theme features that are implemented as custom taxonomies) Also related: 11370-gh-calypso-pre-oss |
d3f2460
to
f224d0e
Compare
|
||
const siteVersion = getSiteOption( state, siteId, 'jetpack_version' ); | ||
if ( ! siteVersion ) { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the use case here where the feature requires 4.1
or higher - won't returning null
here cause a failure down the line in EditorDrawerTaxonomies
since you are explicitly checking for a value of false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't returning
null
here cause a failure down the line inEditorDrawerTaxonomies
since you are explicitly checking for a value offalse
I think at worst it would result in a 404 error on the request, and this could only happen if visiting the editor with a non-primed sites state. The alternative would be to hide the taxonomy accordions for all sites until after we can verify that the site is either not Jetpack or meets the satisfying version.
f224d0e
to
d0ed1b5
Compare
LGMT 🚢 |
Closes #6626
This pull request seeks to display the
<EditorDrawerTaxonomies />
component (custom taxonomies) for all post types. Currently, this is only shown for non-post/page types. However, since posts and pages can have custom taxonomies associated with them, we should render this component.Posts will continue to show Categories & Tags under a single combined accordion, and logic has been included to avoid duplicate accordions from being shown based on the taxonomies associated with the post type.
Testing instructions:
Verify that there are no visual regressions between this branch and master when viewing posts, pages, or a custom post type. Also ensure that if your theme has registered a custom taxonomy associated with a post or page that it is shown in the sidebar of the editor.
Test live: https://calypso.live/?branch=update/cpt-editor-custom-taxonomies