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

Site Logo Block: update Site Logo block UI and option syncing #33179

Merged
merged 5 commits into from
Jul 6, 2021
Merged
Show file tree
Hide file tree
Changes from 2 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
2 changes: 1 addition & 1 deletion lib/init.php
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ function register_site_icon_url( $response ) {
* @return WP_REST_Response
*/
function register_site_logo_to_rest_index( $response ) {
$site_logo_id = get_theme_mod( 'custom_logo' );
$site_logo_id = get_option( 'site_logo' );
creativecoder marked this conversation as resolved.
Show resolved Hide resolved
$response->data['site_logo'] = $site_logo_id;
if ( $site_logo_id ) {
$response->add_link(
Expand Down
86 changes: 49 additions & 37 deletions packages/block-library/src/site-logo/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
ResizableBox,
Spinner,
ToggleControl,
Icon,
Placeholder,
} from '@wordpress/components';
import { useViewportMatch } from '@wordpress/compose';
import {
Expand Down Expand Up @@ -257,37 +257,46 @@ export default function LogoEdit( {
const [ error, setError ] = useState();
const ref = useRef();

const { siteLogoId, canUserEdit, url, mediaItemData } = useSelect(
( select ) => {
const { canUser, getEntityRecord, getEditedEntityRecord } = select(
coreStore
);
const siteSettings = getEditedEntityRecord( 'root', 'site' );
const siteData = getEntityRecord( 'root', '__unstableBase' );
const _siteLogo = siteSettings?.site_logo;
const _readOnlyLogo = siteData?.site_logo;
const _canUserEdit = canUser( 'update', 'settings' );
const _siteLogoId = _siteLogo || _readOnlyLogo;
const mediaItem =
_siteLogoId &&
select( coreStore ).getEntityRecord(
'root',
'media',
_siteLogoId,
{ context: 'view' }
);
return {
siteLogoId: _siteLogoId,
canUserEdit: _canUserEdit,
url: siteData?.url,
mediaItemData: mediaItem && {
url: mediaItem.source_url,
alt: mediaItem.alt_text,
},
};
},
[]
);
const {
siteLogoId,
canUserEdit,
url,
mediaItemData,
isRequestingMediaItem,
} = useSelect( ( select ) => {
const { canUser, getEntityRecord, getEditedEntityRecord } = select(
coreStore
);
const siteSettings = getEditedEntityRecord( 'root', 'site' );
const siteData = getEntityRecord( 'root', '__unstableBase' );
const _siteLogo = siteSettings?.site_logo;
const _readOnlyLogo = siteData?.site_logo;
const _canUserEdit = canUser( 'update', 'settings' );
const _siteLogoId = _siteLogo || _readOnlyLogo;
const mediaItem =
_siteLogoId &&
select( coreStore ).getEntityRecord( 'root', 'media', _siteLogoId, {
context: 'view',
} );
const _isRequestingMediaItem =
_siteLogoId &&
select( coreStore ).isResolving( 'getEntityRecord', [
'root',
'media',
_siteLogoId,
{ context: 'view' },
] );
return {
siteLogoId: _siteLogoId,
canUserEdit: _canUserEdit,
url: siteData?.url,
mediaItemData: mediaItem && {
url: mediaItem.source_url,
alt: mediaItem.alt_text,
},
isRequestingMediaItem: _isRequestingMediaItem,
};
}, [] );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this change is auto-formatting. The only substantial change is adding isRequestingMediaItem.


const { editEntityRecord } = useDispatch( coreStore );
const setLogo = ( newValue ) =>
Expand Down Expand Up @@ -336,7 +345,9 @@ export default function LogoEdit( {

const label = __( 'Site Logo' );
let logoImage;
const isLoading = siteLogoId === undefined || ( siteLogoId && ! logoUrl );
const isLoading =
siteLogoId === undefined ||
( siteLogoId && ! logoUrl && isRequestingMediaItem );
if ( isLoading ) {
logoImage = <Spinner />;
}
Expand Down Expand Up @@ -366,10 +377,11 @@ export default function LogoEdit( {
{ controls }
{ !! logoUrl && logoImage }
{ ! logoUrl && ! canUserEdit && (
<div className="site-logo_placeholder">
<Icon icon={ icon } />
<p> { __( 'Site Logo' ) }</p>
</div>
<Placeholder
className="site-logo_placeholder"
icon={ icon }
label={ label }
/>
) }
{ ! logoUrl && canUserEdit && (
<MediaPlaceholder
Expand Down
27 changes: 5 additions & 22 deletions packages/block-library/src/site-logo/editor.scss
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,19 @@

// Placeholder improvements.
.components-placeholder {
justify-content: flex-start;
min-height: auto;
height: 120px;
padding: $grid-unit-10;
padding: $grid-unit-15;

// Massage the label.
.components-placeholder__label {
margin-top: $grid-unit-15;
white-space: nowrap;
}

.components-placeholder__label .block-editor-block-icon {
.components-placeholder__label .block-editor-block-icon,
.components-placeholder__label > svg {
margin-right: $grid-unit-05;
}

Expand Down Expand Up @@ -80,23 +83,3 @@
}
}
}
.editor-styles-wrapper {
.site-logo_placeholder {
display: flex;
flex-direction: row;
align-items: flex-start;
border-radius: $radius-block-ui;
background-color: $white;
box-shadow: inset 0 0 0 $border-width $gray-900;
padding: $grid-unit-15;
svg {
margin-right: $grid-unit-15;
}
p {
font-family: $default-font;
font-size: $default-font-size;
margin: 0;
line-height: initial;
}
}
}
Comment on lines -83 to -102
Copy link
Contributor Author

@creativecoder creativecoder Jul 2, 2021

Choose a reason for hiding this comment

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

By changing to the <Placeholder> component, these overrides don't seem to be needed.

70 changes: 34 additions & 36 deletions packages/block-library/src/site-logo/index.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
*/
function render_block_core_site_logo( $attributes ) {
$adjust_width_height_filter = function ( $image ) use ( $attributes ) {
if ( empty( $attributes['width'] ) ) {
if ( empty( $attributes['width'] ) || empty( $image ) ) {
Copy link
Contributor Author

@creativecoder creativecoder Jul 2, 2021

Choose a reason for hiding this comment

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

This prevents a PHP warning from the code below if the image was deleted without updating the logo setting.

return $image;
}
$height = (float) $attributes['width'] / ( (float) $image[1] / (float) $image[2] );
Expand Down Expand Up @@ -111,54 +111,52 @@ function _override_custom_logo_theme_mod( $custom_logo ) {
/**
* Updates the site_logo option when the custom_logo theme-mod gets updated.
*
* This function is hooked on "update_option_theme_mods_$theme" and not
* "pre_set_theme_mod_custom_logo" because by hooking in `update_option`
* the function accounts for remove_theme_mod() as well.
*
* @param mixed $old_value The old option value.
* @param mixed $value The new option value.
* @param mixed $value Attachment ID of the custom logo or an empty value.
* @return mixed
*/
function _sync_custom_logo_to_site_logo( $old_value, $value ) {
// Delete the option when the custom logo does not exist or was removed.
// This step ensures the option stays in sync.
if ( empty( $value['custom_logo'] ) ) {
function _sync_custom_logo_to_site_logo( $value ) {
if ( empty( $value ) ) {
delete_option( 'site_logo' );
} else {
remove_action( 'update_option_site_logo', '_sync_site_logo_to_custom_logo' );
update_option( 'site_logo', $value['custom_logo'] );
add_action( 'update_option_site_logo', '_sync_site_logo_to_custom_logo', 10, 2 );
update_option( 'site_logo', $value );
}

return $value;
}

add_filter( 'pre_set_theme_mod_custom_logo', '_sync_custom_logo_to_site_logo' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm restoring the use of the pre_set_theme_mod_custom_logo filter because it solves a couple problems that happen when switching to the update_option_theme_mods_$theme action in #32370.

  • Make sure the site_logo isn't deleted in the case where a theme isn't setting a logo because it doesn't support one
  • Prevent the site_logo from being deleted when switching themes.


/**
* Hooks `_sync_custom_logo_to_site_logo` in `update_option_theme_mods_$theme`.
* Deletes the site_logo when the custom_logo theme mod is removed.
*
* Runs on `setup_theme` to account for dynamically-switched themes in the Customizer.
* @param array $old_value Previous theme mod settings.
* @param array $value Updated theme mod settings.
*/
function _sync_custom_logo_to_site_logo_on_setup_theme() {
$theme = get_option( 'stylesheet' );
add_action( "update_option_theme_mods_$theme", '_sync_custom_logo_to_site_logo', 10, 2 );
function _delete_site_logo_on_remove_custom_logo( $old_value, $value ) {
// If the custom_logo is being unset, it's being removed from theme mods.
if ( isset( $old_value['custom_logo'] ) && ! isset( $value['custom_logo'] ) ) {
delete_option( 'site_logo' );
}
}
add_action( 'setup_theme', '_sync_custom_logo_to_site_logo_on_setup_theme', 11 );

/**
* Updates the custom_logo theme-mod when the site_logo option gets updated.
*
* @param mixed $old_value The old option value.
* @param mixed $value The new option value.
*
* @return void
* Deletes the site logo when all theme mods are being removed.
*/
function _sync_site_logo_to_custom_logo( $old_value, $value ) {
// Delete the option when the custom logo does not exist or was removed.
// This step ensures the option stays in sync.
if ( empty( $value ) ) {
remove_theme_mod( 'custom_logo' );
} else {
remove_filter( 'pre_set_theme_mod_custom_logo', '_sync_custom_logo_to_site_logo' );
set_theme_mod( 'custom_logo', $value );
add_filter( 'pre_set_theme_mod_custom_logo', '_sync_custom_logo_to_site_logo' );
Copy link
Contributor Author

@creativecoder creativecoder Jul 2, 2021

Choose a reason for hiding this comment

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

Note that this line has a bug in it that can delete the site_logo by calling _sync_custom_logo_to_site_logo with just the custom logo value instead of with the theme mods array. If we don't remove this code, we at least need to fix this filter.

function _delete_site_logo_on_remove_theme_mods() {
if ( false !== get_theme_support( 'custom-logo' ) ) {
delete_option( 'site_logo' );
}
}

add_action( 'update_option_site_logo', '_sync_site_logo_to_custom_logo', 10, 2 );
/**
* Hooks `_delete_site_logo_on_remove_custom_logo` in `update_option_theme_mods_$theme`.
* Hooks `_delete_site_logo_on_remove_theme_mods` in `delete_option_theme_mods_$theme`.
*
* Runs on `setup_theme` to account for dynamically-switched themes in the Customizer.
*/
function _delete_site_logo_on_remove_custom_logo_on_setup_theme() {
$theme = get_option( 'stylesheet' );
add_action( "update_option_theme_mods_$theme", '_delete_site_logo_on_remove_custom_logo', 10, 2 );
add_action( "delete_option_theme_mods_$theme", '_delete_site_logo_on_remove_theme_mods', 10, 0 );
creativecoder marked this conversation as resolved.
Show resolved Hide resolved
}
add_action( 'setup_theme', '_delete_site_logo_on_remove_custom_logo_on_setup_theme', 11 );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These hooks and functions cover deleting the site_logo when a custom_logo is removed with remove_theme_mod or remove_theme_mods.

These should allow this change to pass Core phpunit tests (the theme used for the unit tests will need to declare support for custom-logo).