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.

Sign up for GitHub

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

Jump to bottom

Tie zoom out availability to content post supports #66600

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
+60 −10
Open

Tie zoom out availability to content post supports #66600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions docs/reference-guides/data/data-core-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,18 @@ _Returns_

- `string|undefined`: The post type label if available, otherwise undefined.

### getPostTypeSupports
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a selector in the editor package?

Copy link
Contributor Author

@draganescu draganescu Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have not thought about putting it here on purpose, it just came naturally after the getPostTypeLabel selector 🤷🏻

Copy link
Contributor

@youknowriad youknowriad Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see, I was not aware of getPostTypeLabel. I agree that it's similar but it seems to me that neither should exist to be honest (especially in the editor package).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why tho? From the editor package readme:

Having an awareness of the concept of a WordPress post, it associates the loading and saving mechanism of the value representing blocks to a post and its content.

Since it knows of WordPress posts what is wrong with sugar selectors like these?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should avoid "convenient" selectors or shortcuts like that. There's more detailed reasoning here https://make.wordpress.org/core/2024/09/12/gutenberg-development-practices-and-common-pitfalls/

If we allow that, we'll never stop adding shortcuts and APIs everywhere. We just use core-data for these directly.


Returns a post type support object on the current post

_Parameters_

- _state_ `Object`: Global application state.

_Returns_

- `Object`: The post type supports object.

### getPreviousBlockClientId

_Related_
Expand Down
24 changes: 24 additions & 0 deletions lib/init.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,27 @@ function gutenberg_menu() {
);
}
add_action( 'admin_menu', 'gutenberg_menu', 9 );


/**
* This should be implemented in core as a default
* post type feature.
*
*/
if ( ! function_exists( 'add_content_support' ) ) {
/**
* Add the 'content' support to core content post types.
*
* @since 6.8.0
*/
function add_content_support() {
global $wp_post_types;

foreach ( array_keys( $wp_post_types ) as $post_type ) {
if ( in_array( $post_type, array( 'post', 'page', 'wp_template' ), true ) ) {
Copy link
Member

@fabiankaegy fabiankaegy Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I like the idea of this in general. But it seems odd that wp_template has a post type support called content.

In all the other places where we differentiate between wether something is content or now wp_template explicitly counts as non content

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the support type name is debatable b/c of this. It's also still unclear what is content and what is not ... Let's see what other reviewers think.

Copy link
Contributor

Choose a reason for hiding this comment

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

What should the default be for custom post types that plugins might be creating? I.e. is the intention that we're gating the zoom out feature so that plugins will need to opt-in in order to support it?

(I don't have an opinion either way, just a thought as I looked over the PR 🙂)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some kinds of post types where it doesn't make sense. We can make all post types support this by default and opt out only when a custom supports list is provided.

add_post_type_support( $post_type, 'content' );
}
}
}
add_action( 'init', 'add_content_support' );
}
19 changes: 9 additions & 10 deletions packages/editor/src/components/header/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function Header( {
const isLargeViewport = useViewportMatch( 'medium' );
const isTooNarrowForDocumentBar = useMediaQuery( '(max-width: 403px)' );
const {
postType,
postTypeContentSupport,
isTextEditor,
isPublishSidebarOpened,
showIconLabels,
Expand All @@ -66,12 +66,13 @@ function Header( {
const {
getEditorMode,
getEditorSettings,
getCurrentPostType,
isPublishSidebarOpened: _isPublishSidebarOpened,
getPostTypeSupports,
} = select( editorStore );

return {
postType: getCurrentPostType(),
postTypeContentSupport:
getPostTypeSupports().hasOwnProperty( 'content' ),
isTextEditor: getEditorMode() === 'text',
isPublishSidebarOpened: _isPublishSidebarOpened(),
showIconLabels: getPreference( 'core', 'showIconLabels' ),
Expand All @@ -83,10 +84,6 @@ function Header( {
};
}, [] );

const canBeZoomedOut = [ 'post', 'page', 'wp_template' ].includes(
postType
);

const [ isBlockToolsCollapsed, setIsBlockToolsCollapsed ] =
useState( true );

Expand Down Expand Up @@ -149,9 +146,11 @@ function Header( {
<PostSavedState forceIsDirty={ forceIsDirty } />
) }

{ canBeZoomedOut && isEditorIframed && isWideViewport && (
<ZoomOutToggle disabled={ forceDisableBlockTools } />
) }
{ postTypeContentSupport &&
isEditorIframed &&
isWideViewport && (
<ZoomOutToggle disabled={ forceDisableBlockTools } />
) }

<PreviewDropdown
forceIsAutosaveable={ forceIsDirty }
Expand Down
15 changes: 15 additions & 0 deletions packages/editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1814,6 +1814,21 @@ export const getPostTypeLabel = createRegistrySelector(
}
);

/**
* Returns a post type support object on the current post
*
* @param {Object} state Global application state.
*
* @return {Object} The post type supports object.
*/
export const getPostTypeSupports = createRegistrySelector(
( select ) => ( state ) => {
const currentPostType = getCurrentPostType( state );
const postType = select( coreStore ).getPostType( currentPostType );
return postType?.supports ?? {};
}
);
Comment on lines +1817 to +1830
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to introduce a new public registry selector for the value the editor has been deriving forever? 😅

Copy link
Contributor Author

@draganescu draganescu Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I understand we should not add this selector, but what does "the editor has been deriving forever" mean? Do we have this data selectable already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see you mean maybe derrive it from the entity call

const postTypeSupportsComments = useSelect( ( select ) =>
		postType
			? !! select( coreStore ).getPostType( postType )?.supports.comments
			: false
	);

Well we do have a selector above for post title ... IDK!

Copy link
Member

@Mamaduka Mamaduka Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Components like PostTypeSupportCheck just duplicated the logic and derived value from the supports property.

P.S. I didn't meant this as snarky remark, sorry if it came out that way 🙇


/**
* Returns true if the publish sidebar is opened.
*
Expand Down
Loading