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
+92 −95
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion lib/init.php
View file
Open in desktop
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
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 ).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
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.

50 changes: 15 additions & 35 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,34 @@ 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.
* This function is hooked on "pre_update_option_theme_mods_$theme" and not
* "pre_set_theme_mod_custom_logo" because by hooking into the theme_mods
* 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.
* The `pre_update_` hook is used so the `site_logo` option will be updated even
* if the new custom_logo value is the same as the existing one, or the option
* does not exist yet. In both cases the regular `update_` hook is not called.
*
* @param array $value Theme mod settings.
* @return array
*/
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.
function _sync_custom_logo_to_site_logo( $value ) {
if ( empty( $value['custom_logo'] ) ) {
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 );
}

return $value;
}

/**
* Hooks `_sync_custom_logo_to_site_logo` in `update_option_theme_mods_$theme`.
* Hooks `_sync_custom_logo_to_site_logo` into `pre_update_option_theme_mods_$theme`.
*
* Runs on `setup_theme` to account for dynamically-switched themes in the Customizer.
*/
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 );
add_filter( "pre_update_option_theme_mods_$theme", '_sync_custom_logo_to_site_logo', 10 );
}
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
*/
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.

}
}

add_action( 'update_option_site_logo', '_sync_site_logo_to_custom_logo', 10, 2 );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see the PR description for why I'm proposing to remove this.