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: Add option to set site icon from Site Logo block #35892

Merged
merged 19 commits into from
Dec 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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 docs/reference-guides/core-blocks.md
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,7 @@ Display a graphic to represent this site. Update the block, and the changes appl
- **Name:** core/site-logo
- **Category:** theme
- **Supports:** align, color (~~background~~, ~~text~~), ~~alignWide~~, ~~html~~
- **Attributes:** isLink, linkTarget, width
- **Attributes:** isLink, linkTarget, shouldSyncIcon, width

## Site Tagline

Expand Down
6 changes: 5 additions & 1 deletion packages/block-library/src/site-logo/block.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,16 @@
"linkTarget": {
"type": "string",
"default": "_self"
},
"shouldSyncIcon": {
"type": "boolean"
}
},
"example": {
"viewportWidth": 500,
"attributes": {
"width": 350
"width": 350,
"className": "block-editor-block-types-list__site-logo-example"
}
},
"supports": {
Expand Down
131 changes: 124 additions & 7 deletions packages/block-library/src/site-logo/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,12 @@ import { includes, pick } from 'lodash';
* WordPress dependencies
*/
import { isBlobURL } from '@wordpress/blob';
import { useEffect, useState, useRef } from '@wordpress/element';
import {
createInterpolateElement,
useEffect,
useState,
useRef,
} from '@wordpress/element';
import { __, isRTL } from '@wordpress/i18n';
import {
MenuItem,
Expand Down Expand Up @@ -53,14 +58,17 @@ const ACCEPT_MEDIA_STRING = 'image/*';

const SiteLogo = ( {
alt,
attributes: { align, width, height, isLink, linkTarget },
attributes: { align, width, height, isLink, linkTarget, shouldSyncIcon },
containerRef,
isSelected,
setAttributes,
setLogo,
logoUrl,
siteUrl,
logoId,
iconId,
setIcon,
canUserEdit,
} ) => {
const clientWidth = useClientWidth( containerRef, [ align ] );
const isLargeViewport = useViewportMatch( 'medium' );
Expand All @@ -84,6 +92,15 @@ const SiteLogo = ( {
};
}, [] );

useEffect( () => {
// Turn the `Use as site icon` toggle off if it is on but the logo and icon have
// fallen out of sync. This can happen if the toggle is saved in the `on` position,
// but changes are later made to the site icon in the Customizer.
if ( shouldSyncIcon && logoId !== iconId ) {
setAttributes( { shouldSyncIcon: false } );
}
}, [] );

useEffect( () => {
if ( ! isSelected ) {
setIsEditingImage( false );
Expand Down Expand Up @@ -250,6 +267,25 @@ const SiteLogo = ( {
</ResizableBox>
);

const syncSiteIconHelpText = createInterpolateElement(
__(
'Site Icons are what you see in browser tabs, bookmark bars, and within the WordPress mobile apps. To use a custom icon that is different from your site logo, use the <a>Site Icon settings</a>.'
),
Copy link
Member

Choose a reason for hiding this comment

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

I have personal preference for less copy, especially seeing as the Site Logo block description covers a lot already, so this suggestion is 100% ignorable 😄

Toggle to set your logo to also be your icon. You can upload a custom site icon in the <a>Customizer</a>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm I was concerned with making sure that the distinction between icon and logo was clear here. You make a good point about the Site Logo block description -- I hadn't noticed it addresses the site icon in that text as well:

Display a graphic to represent this site. Update the block, and the changes apply everywhere it’s used. This is different than the site icon, which is the smaller image visible in your dashboard, browser tabs, etc used to help others recognize this site.

Knowing that it does feel a bit redundant, but I still think it's important to make sure the use of the toggle is clear from its own helptext, and I imagine not everyone will read through the block description 🤔

{
a: (
// eslint-disable-next-line jsx-a11y/anchor-has-content
<a
href={
siteUrl +
'/wp-admin/customize.php?autofocus[section]=title_tagline'
}
target="_blank"
rel="noopener noreferrer"
/>
),
}
);

return (
<>
<InspectorControls>
Expand Down Expand Up @@ -286,6 +322,19 @@ const SiteLogo = ( {
/>
</>
) }
{ canUserEdit && (
<>
<ToggleControl
label={ __( 'Use as site icon' ) }
onChange={ ( value ) => {
setAttributes( { shouldSyncIcon: value } );
setIcon( value ? logoId : undefined );
} }
checked={ !! shouldSyncIcon }
help={ syncSiteIconHelpText }
/>
</>
) }
</PanelBody>
</InspectorControls>
<BlockControls group="block">
Expand All @@ -308,14 +357,15 @@ export default function LogoEdit( {
setAttributes,
isSelected,
} ) {
const { width } = attributes;
const { className: styleClass, width, shouldSyncIcon } = attributes;
const [ logoUrl, setLogoUrl ] = useState();
const ref = useRef();

const {
siteLogoId,
canUserEdit,
url,
siteIconId,
mediaItemData,
isRequestingMediaItem,
} = useSelect( ( select ) => {
Expand All @@ -328,6 +378,7 @@ export default function LogoEdit( {
const _readOnlyLogo = siteData?.site_logo;
const _canUserEdit = canUser( 'update', 'settings' );
const _siteLogoId = _canUserEdit ? _siteLogo : _readOnlyLogo;
const _siteIconId = siteSettings?.site_icon;
const mediaItem =
_siteLogoId &&
select( coreStore ).getMedia( _siteLogoId, {
Expand All @@ -339,6 +390,7 @@ export default function LogoEdit( {
_siteLogoId,
{ context: 'view' },
] );

return {
siteLogoId: _siteLogoId,
canUserEdit: _canUserEdit,
Expand All @@ -349,14 +401,59 @@ export default function LogoEdit( {
alt: mediaItem.alt_text,
},
isRequestingMediaItem: _isRequestingMediaItem,
siteIconId: _siteIconId,
};
}, [] );

const { getGlobalBlockCount } = useSelect( blockEditorStore );
const { editEntityRecord } = useDispatch( coreStore );
const setLogo = ( newValue ) =>

useEffect( () => {
Copy link
Contributor

@youknowriad youknowriad Jan 10, 2022

Choose a reason for hiding this comment

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

Hi there!

The logic here doesn't make sense, it's creating bugs like #37760

Basically the logic here assumes than when we "unmount" the site logo block, we're removing it when it's not always the case, unmounting can happen in several ways:

  • Switching to code editor in post editor
  • Changing the current view (template/template part) in the site editor.

While I'm not sure about the rest of this PR, this effect should be removed.

Copy link
Member

@ramonjd ramonjd Jan 11, 2022

Choose a reason for hiding this comment

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

@stacimc I removed the hook and went through the testing instructions above under "Test discarding unsaved changes:"

I couldn't find any weird side effects, that is, the editor recognised and kept my updates when viewing previews and when switching between code/visual modes.

In other words, things still work for me.

Is there something I'm not testing right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic here doesn't assume that unmounting the block means we're removing it. It runs every time the block is unmounted, but it then checks the global block count to determine whether the Site Logo blocks have actually been removed; otherwise it should do nothing.

I think the problem is in the Site Editor, that getGlobalBlockCount isn't behaving the way I believed it would. Although all my own test scenarios still work for me, I'm able to reproduce the #37760 bug by:

  • Add a Site Logo block and make a change in the Header part
  • Add a different block to a different template part which does not contain a Site Logo block
  • Hit Undo and see that the 'Redo' button is disabled

When the Site logo block unmounts, getGlobalBlockCount returns 0 this time.

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'll put up a PR to remove the logic for now and then start looking into a better way of doing this. I do think it would be nice to discard those unsaved changes when the block is removed, if I can get it working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#37895 fixes the regression, and #37900 has been opened to investigate adding the functionality back in safely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @stacimc getGlobalBlockCount is not reliable because the core/block-editor store is more like an internal store to an instance of the block editor and don't say anything about the presence of a block in a template (or all templates). It's just specific to the current state of the screen.

// Cleanup function to discard unsaved changes to the icon and logo when
// the block is removed.
return () => {
// Do nothing if the block is being rendered in the styles preview or the
// block inserter.
if (
styleClass?.includes(
'block-editor-block-types-list__site-logo-example'
) ||
styleClass?.includes(
'block-editor-block-styles__block-preview-container'
)
) {
return;
}

const logoBlockCount = getGlobalBlockCount( 'core/site-logo' );

// Only discard unsaved changes if we are removing the last Site Logo block
// on the page.
if ( logoBlockCount === 0 ) {
editEntityRecord( 'root', 'site', undefined, {
site_logo: undefined,
site_icon: undefined,
} );
}
};
}, [] );

const setLogo = ( newValue, shouldForceSync = false ) => {
// `shouldForceSync` is used to force syncing when the attribute
// may not have updated yet.
if ( shouldSyncIcon || shouldForceSync ) {
setIcon( newValue );
}

editEntityRecord( 'root', 'site', undefined, {
site_logo: newValue,
} );
};

const setIcon = ( newValue ) =>
editEntityRecord( 'root', 'site', undefined, {
site_icon: newValue,
} );

let alt = null;
if ( mediaItemData ) {
Expand All @@ -365,7 +462,24 @@ export default function LogoEdit( {
setLogoUrl( mediaItemData.url );
}
}
const onSelectLogo = ( media ) => {

const onInitialSelectLogo = ( media ) => {
// Initialize the syncSiteIcon toggle. If we currently have no Site logo and no
// site icon, automatically sync the logo to the icon.
if ( shouldSyncIcon === undefined ) {
const shouldForceSync = ! siteIconId;
setAttributes( { shouldSyncIcon: shouldForceSync } );

// Because we cannot rely on the `shouldSyncIcon` attribute to have updated by
// the time `setLogo` is called, pass an argument to force the syncing.
onSelectLogo( media, shouldForceSync );
return;
}

onSelectLogo( media );
};

const onSelectLogo = ( media, shouldForceSync = false ) => {
if ( ! media ) {
return;
}
Expand All @@ -377,7 +491,7 @@ export default function LogoEdit( {
return;
}

setLogo( media.id );
setLogo( media.id, shouldForceSync );
};

const onRemoveLogo = () => {
Expand Down Expand Up @@ -423,6 +537,9 @@ export default function LogoEdit( {
setLogo={ setLogo }
logoId={ mediaItemData?.id || siteLogoId }
siteUrl={ url }
setIcon={ setIcon }
iconId={ siteIconId }
canUserEdit={ canUserEdit }
/>
);
}
Expand Down Expand Up @@ -481,7 +598,7 @@ export default function LogoEdit( {
) }
{ ! logoUrl && canUserEdit && (
<MediaPlaceholder
onSelect={ onSelectLogo }
onSelect={ onInitialSelectLogo }
accept={ ACCEPT_MEDIA_STRING }
allowedTypes={ ALLOWED_MEDIA_TYPES }
onError={ onUploadError }
Expand Down
17 changes: 17 additions & 0 deletions packages/block-library/src/site-logo/index.php
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,23 @@ function register_block_core_site_logo_setting() {

add_action( 'rest_api_init', 'register_block_core_site_logo_setting', 10 );

/**
* Register a core site setting for a site icon
*/
function register_block_core_site_icon_setting() {
register_setting(
'general',
'site_icon',
array(
'show_in_rest' => true,
'type' => 'integer',
'description' => __( 'Site icon.' ),
)
);
}

add_action( 'rest_api_init', 'register_block_core_site_icon_setting', 10 );

/**
* Registers the `core/site-logo` block on the server.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const TRANSLATED_SITE_PROPERTIES = {
title: __( 'Title' ),
description: __( 'Tagline' ),
site_logo: __( 'Logo' ),
site_icon: __( 'Icon' ),
show_on_front: __( 'Show on front' ),
page_on_front: __( 'Page on front' ),
};
Expand Down