-
Notifications
You must be signed in to change notification settings - Fork 4.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
Fix Navigation Links when post type is not Page or Post #28892
Conversation
2e525bc
to
16d5e38
Compare
Size Change: 0 B Total Size: 1.37 MB ℹ️ View Unchanged
|
16d5e38
to
8ac6f25
Compare
258e1e3
to
e1badf9
Compare
if ( isset( $attributes['id'] ) && is_numeric( $attributes['id'] ) ) { | ||
if ( | ||
isset( $attributes['id'] ) && is_numeric( $attributes['id'] ) && | ||
( 'post' === $attributes['type'] || 'page' === $attributes['type'] ) |
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.
I wonder here if we should exclude what get_post
does not support (tags, categories) because CPT's (e.g. a book
) should also follow this draft logic but we can't add them all to this condition.
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.
That is a good point @draganescu. Thanks for taking a look! How about we follow up on it in the next PR, since I'm working on #24814 which adds variations for CPTs and taxonomies?
At the moment I'm not sure type
is enough to encapsulate what something is. It looks like we're using the post-type and taxonomy slug as a type value, and I'm not sure we enforce uniqueness across the two strings. For example can someone create a poorly named custom taxonomy that collides with a custom post type? I'll need to experiment. We'll also need to respect show_in_nav_menus
on both entities.
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.
As long as this remains on the radar, I am merging this 🚀
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.
Apart from this comment I think this is good to go.
I'm now getting an error on posts with Navigation blocks in them (both in editor and front end): |
I noticed that too, fix here - #28999. |
This adds a check in the navigation link render callback to verify if a link is a post, before checking publish status.
It doesn't look like we have any other examples of this sort of dynamic block test, so I tried to scope the unit as low as possible. Let me know if folks had preferences.
Before
categorypostidoverlap.mp4
After
Manual Testing Instructions
Verify that tests pass:
run
npm run test-unit-php
Fixes #28712