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

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

Merged
merged 5 commits into from
Jul 6, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
de637ab
Improve and simplify Site Logo block UI and option syncing
creativecoder Jul 2, 2021
f528c4d
Update site logo filters to fix logo removal when switching themes
creativecoder Jul 4, 2021
bb8f36c
Revert API change
creativecoder Jul 5, 2021
ea36e79
Remove action args
creativecoder Jul 5, 2021
a5e9b87
Update loading logic for smoother UI transitions
creativecoder Jul 5, 2021
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
90 changes: 53 additions & 37 deletions packages/block-library/src/site-logo/edit.js
View file
Open in desktop
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 ).hasFinishedResolution( '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,
};
}, [] );

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

const label = __( 'Site Logo' );
let logoImage;
const isLoading = siteLogoId === undefined || ( siteLogoId && ! logoUrl );
const isLoading = siteLogoId === undefined || isRequestingMediaItem;
if ( isLoading ) {
logoImage = <Spinner />;
}
Expand Down Expand Up @@ -366,10 +375,17 @@ 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 }
>
{ isLoading && (
<span className="components-placeholder__preview">
<Spinner />
</span>
) }
</Placeholder>
) }
{ ! logoUrl && canUserEdit && (
<MediaPlaceholder
Expand Down
27 changes: 5 additions & 22 deletions packages/block-library/src/site-logo/editor.scss
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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 •
edited
Loading

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 •
edited
Loading

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' );
}
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).