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

Missing Post Type supports in Options Modal #12394

Merged
merged 7 commits into from
Jan 18, 2019

Conversation

imath
Copy link
Contributor

@imath imath commented Nov 28, 2018

  • Featured Image is not only a Theme thing, it is possible for a custom post type to declare itself without supporting the thumbnail feature.
  • A custom post type can also register itself withour supporting comments and/or custom-fields.

As a result the corresponding Document panels in the editor sidebar are not displayed. The Options Modal should also avoid displaying the panel checkboxes if the custom post type does not support these 3 features

Description

To fix this issue, i've wrapped featured image, comments and custom fields checkboxes into <PostTypeSupportCheck/> components

How has this been tested?

I've tested the PR with the wp_block post type which only supports the title and editor features and check everything was still behaving the right way with the post post type.

Screenshots

Without the PR applied:

without

Checkboxes for featured image, comments, and custom fields are displayed although they shouldn't

With the PR applied:

with

The checkboxes for these 3 unsupported features are no more displayed.

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Copy link
Member

@ajitbohra ajitbohra left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ajitbohra ajitbohra requested a review from gziolo November 29, 2018 16:48
@gziolo gziolo added this to the 4.7 milestone Nov 30, 2018
@gziolo gziolo added [Feature] Extensibility The ability to extend blocks or the editing experience [Type] Enhancement A suggestion for improvement. [Type] Bug An existing feature does not function as intended and removed [Type] Enhancement A suggestion for improvement. labels Nov 30, 2018
@gziolo
Copy link
Member

gziolo commented Nov 30, 2018

Should we also wrap the permalink section with check? It Was added in #12132.

@imath imath force-pushed the fix/options-modal-featured-image branch from ff89e74 to 6916132 Compare December 1, 2018 03:34
@imath
Copy link
Contributor Author

imath commented Dec 1, 2018

@gziolo Absolutely. Thanks for mentioning it. I've just updated the PR accordingly.

  • I've updated the Options Modal snapshots that were failing.

@noisysocks
Copy link
Member

It would be nice if we didn't display the section title when there are no options underneath it. This came up in #11802 (comment) as well.

@gziolo
Copy link
Member

gziolo commented Dec 3, 2018

It would be nice if we didn't display the section title when there are no options underneath it. This came up in #11802 (comment) as well.

Yes, I opened #11923 to track it.

@imath imath force-pushed the fix/options-modal-featured-image branch 2 times, most recently from beda76c to 20d1669 Compare December 5, 2018 15:24
@imath
Copy link
Contributor Author

imath commented Dec 5, 2018

@gziolo in 20d1669 I've succeeded to avoid the section title display, but I haven't find a better way than using a huge check 😫

Here's a screenshot:

nosections

@noisysocks
Copy link
Member

Let's tackle #11923 in a seperate PR. I don't think it's urgent.

@imath
Copy link
Contributor Author

imath commented Dec 6, 2018

Fine with me @noisysocks, should I revert 20d1669 @gziolo ?

@gziolo
Copy link
Member

gziolo commented Dec 14, 2018

Fine with me @noisysocks, should I revert 20d1669 @gziolo ?

Yes, I would put it in a follow-up PR so we could iterate on it 👍

lib/client-assets.php Show resolved Hide resolved
const hasPageAttributeSupport = get( postType, [ 'supports', 'page-attributes' ], false ) || hasTemplates;
let documentPanelsSection;

if ( isViewable || hasTaxonomies || hasFeaturedImageSupport || hasExcerptSupport || hasCommentsSupport || hasPageAttributeSupport ) {
Copy link
Member

Choose a reason for hiding this comment

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

It's going to be a more tricky check. All those panels can be also removed programmatically :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😱

@imath imath force-pushed the fix/options-modal-featured-image branch from 20d1669 to f295ac5 Compare December 15, 2018 08:36
@imath
Copy link
Contributor Author

imath commented Dec 15, 2018

@gziolo I've just reverted the commit that was doing the huge check to try to solve #11923

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Looks good after all changes made 👍

@gziolo
Copy link
Member

gziolo commented Dec 17, 2018

@noisysocks - do you want to perform another round of testing before we proceed? It's blocked by your review at the moment :)

@noisysocks noisysocks dismissed their stale review December 19, 2018 04:36

No bandwidth to re-review right now

@noisysocks
Copy link
Member

@gziolo: Dismissed my stale review. Am doing a support rotation this week so best not wait for me 🙂

@youknowriad youknowriad modified the milestones: 4.8, 4.9 Dec 19, 2018
@gziolo
Copy link
Member

gziolo commented Dec 19, 2018

@imath - can you rebase with master? Travis is back to normal :)

Featured Image is not only a Theme thing, it is possible for a custom post type to declare itself without supporting the `thumbnail` feature.
A custom post type can also register itself withour supporting `comments` and/or `custom-fields`.
As a result the corresponding Document panels in the editor sidebar are not displayed. The Options Modal should also avoid displaying the panel checkboxes if the custom post type does not support these 3 features
As recommanded by @noisysocks : make sure the behavior about custom fields is consistent between WordPress 5.0 RC and the Gutenberg plugin
It should fix the failing test.
@imath imath force-pushed the fix/options-modal-featured-image branch from f295ac5 to 3e360ae Compare December 19, 2018 13:44
@imath
Copy link
Contributor Author

imath commented Dec 19, 2018

@gziolo Sure, I've just rebased the PR 😉

@youknowriad youknowriad merged commit e5f398c into WordPress:master Jan 18, 2019
@imath
Copy link
Contributor Author

imath commented Jan 18, 2019

@youknowriad sorry i’ve clicked on the resolve button before posting this comment.

There’s no need to backport as it is already there these lines simply moved down a few lines. You should see the removed lines highlighted in red into the changes.

@imath
Copy link
Contributor Author

imath commented Jan 18, 2019

And btw thanks a lot for the merge 😍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants