Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
3 changes: 3 additions & 0 deletions packages/block-library/src/cover/edit/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -703,10 +703,13 @@ function CoverEdit( {
setOverlayColor={ onSetOverlayColor }
coverRef={ ref }
currentSettings={ currentSettings }
onSelectMedia={ onSelectMedia }
onUploadError={ onUploadError }
toggleUseFeaturedImage={ toggleUseFeaturedImage }
updateDimRatio={ onUpdateDimRatio }
onClearMedia={ onClearMedia }
featuredImage={ media }
isSelected={ isSelected }
/>
);

Expand Down
72 changes: 71 additions & 1 deletion packages/block-library/src/cover/edit/inspector-controls.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,13 @@
import { __ } from '@wordpress/i18n';
import { useSelect } from '@wordpress/data';
import { store as coreStore } from '@wordpress/core-data';
import { getFilename } from '@wordpress/url';
import { Link } from '@wordpress/ui';
import { COVER_MIN_HEIGHT, mediaPosition } from '../shared';
import {
ALLOWED_MEDIA_TYPES,
COVER_MIN_HEIGHT,
mediaPosition,
} from '../shared';
import { unlock } from '../../lock-unlock';
import { useToolsPanelDropdownMenuProps } from '../../utils/hooks';
import {
Expand All @@ -35,6 +40,7 @@
} from '../../utils/style-state';
import { DEFAULT_MEDIA_SIZE_SLUG } from '../constants';
import PosterImage from '../../utils/poster-image';
import { MediaControl } from '../../utils/media-control';

const {
cleanEmptyObject,
Expand Down Expand Up @@ -98,8 +104,13 @@
setOverlayColor,
coverRef,
currentSettings,
onSelectMedia,
onUploadError,
toggleUseFeaturedImage,
updateDimRatio,
onClearMedia,
featuredImage,
isSelected,
} ) {
const {
useFeaturedImage,
Expand Down Expand Up @@ -138,6 +149,24 @@
},
[ clientId ]
);
/*
* The block only resolves the featured image record, so the record of a
* media selection of its own is resolved here to name it in the panel.
*/
const selectedMedia = useSelect(
( select ) =>
id && ! useFeaturedImage
? select( coreStore ).getEntityRecord(
'postType',
'attachment',
id,
{ context: 'view' }
)

Check failure on line 164 in packages/block-library/src/cover/edit/inspector-controls.jsx

View workflow job for this annotation

GitHub Actions / All

Replace `··` with `↹`
: undefined,
[ id, useFeaturedImage ]
);
const mediaRecord = useFeaturedImage ? featuredImage : selectedMedia;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks pretty much the same as the code on 197-210, so I think it might be a mistake.

The conditions are slightly different (isImageBackground instead of useFeaturedImage), but I think it makes little difference practically.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems other blocks don't display a preview for the featured image here. Is there a reason we're handling this differently?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@talldan Good catch, removed the duplicate fetch and reused the existing one.

@Mamaduka No good reason, it now matches the other blocks. Fixed it.


const hasSelectedStyleState =
! isDefaultBlockStyleState( selectedStyleState );
const selectedStyleStateKey = getStyleStateKey( selectedStyleState );
Expand Down Expand Up @@ -277,8 +306,49 @@

const dropdownMenuProps = useToolsPanelDropdownMenuProps();

/*
* Rendered whether or not media has been set, so that a cover in its setup
* state can be given media from the inspector as well as from the toolbar.
* The settings below stay behind their own condition, having nothing to
* configure until there is media to configure.
*/
const mediaInspectorPanel = (
<InspectorControls group="content">
<ToolsPanel
label={ __( 'Media' ) }
resetAll={ onClearMedia }
dropdownMenuProps={ dropdownMenuProps }
>
<ToolsPanelItem
label={ __( 'Media' ) }
hasValue={ () => !! url || !! useFeaturedImage }
onDeselect={ onClearMedia }
isShownByDefault
>
<MediaControl
mediaId={ id }
mediaUrl={ url }
filename={
mediaRecord?.media_details?.sizes?.full?.file ||
mediaRecord?.slug ||
getFilename( url )
}
allowedTypes={ ALLOWED_MEDIA_TYPES }
onSelect={ onSelectMedia }
onError={ onUploadError }
onReset={ onClearMedia }
useFeaturedImage={ useFeaturedImage }
onToggleFeaturedImage={ toggleUseFeaturedImage }
emptyLabel={ __( 'Add media' ) }
/>
</ToolsPanelItem>
</ToolsPanel>
</InspectorControls>
);

return (
<>
{ isSelected && mediaInspectorPanel }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the same change is required as the other PR, either inline the mediaInspectorPanel JSX, or move the isSelected conditional to line 315.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, moved the selection check onto the panel itself, same as Media & Text.

{ ( !! url || useFeaturedImage ) && (
<InspectorControls>
<ToolsPanel
Expand Down
20 changes: 18 additions & 2 deletions packages/block-library/src/cover/test/edit.browser.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,16 @@ async function openStylesTabIfAvailable() {
}
}

async function openSettingsTabIfAvailable() {
const settingsTab = screen.queryByRole( 'tab', {
name: 'Settings',
} );

if ( settingsTab ) {
await userEvent.click( settingsTab );
}
}

describe( 'Cover block', () => {
beforeEach( () => registerCoreBlocks() );

Expand Down Expand Up @@ -207,11 +217,12 @@ describe( 'Cover block', () => {
} )
).not.toBeInTheDocument();
} );
test( 'does not display settings tab when media settings are empty', async () => {
test( 'does not display media settings panel when media settings are empty', async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's confusing because this PR adds a Media panel in the content tab, but this test is referring to just the 'Settings' panel, so I suggest changing it to this

Suggested change
test( 'does not display media settings panel when media settings are empty', async () => {
test( 'does not display settings panel in the settings tab when the block has no media', async () => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, renamed it as suggested.

await createAndSelectBlock();

await openSettingsTabIfAvailable();
expect(
screen.queryByRole( 'tab', {
screen.queryByRole( 'heading', {
name: 'Settings',
} )
).not.toBeInTheDocument();
Expand All @@ -227,6 +238,7 @@ describe( 'Cover block', () => {
} );

await selectBlock( 'Block: Cover' );
await openSettingsTabIfAvailable();
expect(
await screen.findByRole( 'heading', { name: 'Settings' } )
).toBeInTheDocument();
Expand All @@ -241,6 +253,7 @@ describe( 'Cover block', () => {
'has-parallax'
);
await selectBlock( 'Block: Cover' );
await openSettingsTabIfAvailable();
await userEvent.click(
await screen.findByLabelText( 'Fixed background' )
);
Expand All @@ -257,6 +270,7 @@ describe( 'Cover block', () => {
'is-repeated'
);
await selectBlock( 'Block: Cover' );
await openSettingsTabIfAvailable();
await userEvent.click(
await screen.findByLabelText( 'Repeated background' )
);
Expand All @@ -271,6 +285,7 @@ describe( 'Cover block', () => {
} );

await selectBlock( 'Block: Cover' );
await openSettingsTabIfAvailable();
await userEvent.clear( await screen.findByLabelText( 'Left' ) );
await userEvent.type( screen.getByLabelText( 'Left' ), '100' );

Expand All @@ -288,6 +303,7 @@ describe( 'Cover block', () => {
} );

await selectBlock( 'Block: Cover' );
await openSettingsTabIfAvailable();
await userEvent.type(
await screen.findByLabelText( 'Alternative text' ),
'Me'
Expand Down
3 changes: 3 additions & 0 deletions test/e2e/specs/editor/blocks/cover.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,9 @@ test.describe( 'Cover', () => {

await editor.selectBlocks( coverBlock );

// The focal point picker is in the Settings tab of the block inspector.
await page.getByRole( 'tab', { name: 'Settings' } ).click();

// The focal point picker renders as a `legend`-labelled group, so both
// spinbuttons are resolved through it.
const focalPointGroup = page.getByRole( 'group', {
Expand Down
Loading