Editor: Display Site Icon (if one is set) in Gutenberg Fullscreen Mode #22952
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,37 +7,64 @@ import { get } from 'lodash'; | |
| * WordPress dependencies | ||
| */ | ||
| import { useSelect } from '@wordpress/data'; | ||
| import { Button, Icon } from '@wordpress/components'; | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { addQueryArgs } from '@wordpress/url'; | ||
| import { wordpress } from '@wordpress/icons'; | ||
|
|
||
| function FullscreenModeClose() { | ||
| const { isActive, isRequestingSiteIcon, postType, siteIconUrl } = useSelect( | ||
| ( select ) => { | ||
| const { getCurrentPostType } = select( 'core/editor' ); | ||
| const { isFeatureActive } = select( 'core/edit-post' ); | ||
| const { isResolving } = select( 'core/data' ); | ||
| const { getEntityRecord, getPostType } = select( 'core' ); | ||
| const siteData = | ||
| getEntityRecord( 'root', '__unstableBase', undefined ) || {}; | ||
|
|
||
| return { | ||
|
There was a problem hiding this comment. @mtias @youknowriad do you think we should limit this to just edit-site for now, or do you think it would also be good to start testing it in edit-post? There was a problem hiding this comment. We should do it in both, but if we are not certain yet, better to keep it in edit-site. We would need to decide if it's ready for 5.5 as well (in edit-post). There was a problem hiding this comment.
Sounds good 👍 . I'm newer on the team so I'm not sure what this entails. Could you elaborate? Also, I wanted to call out that I'm on the gardening rotation so development on this PR is going to slow down until the end of the month. There was a problem hiding this comment. The second week of July is the beta period for WordPress 5.5. Anything that affects the post-editor needs to be in good shape by then. There was a problem hiding this comment. @mtias I'm back from the gardening rotation, which leaves us a few weeks to test this feature if we want it in 5.5. I see two options here: 1. Test post-editor for 5.5 releasePros:
Cons:
2. Test post-editor for 5.6 releasePros:
Cons:
Also @sirreal Would you be able to send me documentation about how I'd make sure the post-editor is tested thoroughly with these changes for the 5.5 beta? There was a problem hiding this comment. @mtias @vindl Since it sounds like there is more extensive testing needed before releasing this feature in Since the work here isn't urgent, we wouldn't have to rush testing while we prep for 5.6. Let me know what you think. |
||
| isActive: isFeatureActive( 'fullscreenMode' ), | ||
| isRequestingSiteIcon: isResolving( 'core', 'getEntityRecord', [ | ||
| 'root', | ||
| '__unstableBase', | ||
| undefined, | ||
| ] ), | ||
| postType: getPostType( getCurrentPostType() ), | ||
| siteIconUrl: siteData.site_icon_url, | ||
| }; | ||
| }, | ||
| [] | ||
| ); | ||
|
|
||
| if ( ! isActive || ! postType ) { | ||
| return null; | ||
| } | ||
|
|
||
| let buttonIcon = <Icon size="36px" icon={ wordpress } />; | ||
|
|
||
| if ( siteIconUrl ) { | ||
| buttonIcon = ( | ||
| <img | ||
| alt={ __( 'Site Icon' ) } | ||
| className="edit-post-fullscreen-mode-close_site-icon" | ||
| src={ siteIconUrl } | ||
| /> | ||
| ); | ||
| } else if ( isRequestingSiteIcon ) { | ||
| buttonIcon = null; | ||
Copons marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| return ( | ||
| <Button | ||
| className="edit-post-fullscreen-mode-close has-icon" | ||
|
There was a problem hiding this comment. I might be missing some context, but is there a reason why we're adding the There was a problem hiding this comment. Good question. Previously, we used to pass the Since we're no longer passing the logo to However, we might explore an alternative... There was a problem hiding this comment. The There was a problem hiding this comment. I was experimenting a bit with this, and it would be possible. However, the |
||
| href={ addQueryArgs( 'edit.php', { | ||
| post_type: postType.slug, | ||
| } ) } | ||
| label={ get( postType, [ 'labels', 'view_items' ], __( 'Back' ) ) } | ||
jeyip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| showTooltip | ||
| > | ||
| { buttonIcon } | ||
| </Button> | ||
| ); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,3 +27,8 @@ | |
| } | ||
| } | ||
| } | ||
|
|
||
| .edit-post-fullscreen-mode-close_site-icon { | ||
| width: 36px; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| /** | ||
| * External dependencies | ||
| */ | ||
| import { render } from '@testing-library/react'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useSelect } from '@wordpress/data'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import FullscreenModeClose from '../'; | ||
|
|
||
| jest.mock( '@wordpress/data/src/components/use-select', () => { | ||
| // This allows us to tweak the returned value on each test | ||
| const mock = jest.fn(); | ||
| return mock; | ||
| } ); | ||
|
|
||
| jest.mock( '@wordpress/core-data' ); | ||
|
|
||
| describe( 'FullscreenModeClose', () => { | ||
| describe( 'when in full screen mode', () => { | ||
| it( 'should display a user uploaded site icon if it exists', () => { | ||
| useSelect.mockImplementation( ( cb ) => { | ||
| return cb( () => ( { | ||
| isResolving: () => false, | ||
| isFeatureActive: () => true, | ||
| getCurrentPostType: () => {}, | ||
| getPostType: () => true, | ||
| getEntityRecord: () => ( { | ||
| site_icon_url: 'https://fakeUrl.com', | ||
| } ), | ||
| } ) ); | ||
| } ); | ||
|
|
||
| const { container } = render( <FullscreenModeClose /> ); | ||
| const siteIcon = container.querySelector( | ||
| '.edit-post-fullscreen-mode-close_site-icon' | ||
| ); | ||
|
|
||
| expect( siteIcon ).toBeTruthy(); | ||
| } ); | ||
|
|
||
| it( 'should display a default site icon if no user uploaded site icon exists', () => { | ||
| useSelect.mockImplementation( ( cb ) => { | ||
| return cb( () => ( { | ||
| isResolving: () => false, | ||
| isFeatureActive: () => true, | ||
| getCurrentPostType: () => {}, | ||
| getPostType: () => true, | ||
| getEntityRecord: () => ( { | ||
| site_icon_url: '', | ||
| } ), | ||
| } ) ); | ||
| } ); | ||
|
|
||
| const { container } = render( <FullscreenModeClose /> ); | ||
| const siteIcon = container.querySelector( | ||
| '.edit-post-fullscreen-mode-close_site-icon' | ||
| ); | ||
| const defaultIcon = container.querySelector( 'svg' ); | ||
|
|
||
| expect( siteIcon ).toBeFalsy(); | ||
| expect( defaultIcon ).toBeTruthy(); | ||
| } ); | ||
| } ); | ||
| } ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| /** | ||
| * External dependencies | ||
| */ | ||
| import { render } from '@testing-library/react'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useSelect } from '@wordpress/data'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import FullscreenModeClose from '../'; | ||
|
|
||
| jest.mock( '@wordpress/data/src/components/use-select', () => { | ||
| // This allows us to tweak the returned value on each test | ||
| const mock = jest.fn(); | ||
| return mock; | ||
| } ); | ||
|
|
||
| jest.mock( '@wordpress/core-data' ); | ||
|
|
||
| describe( 'FullscreenModeClose', () => { | ||
| describe( 'when in full screen mode', () => { | ||
| it( 'should display a user uploaded site icon if it exists', () => { | ||
| useSelect.mockImplementation( ( cb ) => { | ||
| return cb( () => ( { | ||
| isResolving: () => false, | ||
| isFeatureActive: () => true, | ||
| getEntityRecord: () => ( { | ||
| site_icon_url: 'https://fakeUrl.com', | ||
| } ), | ||
| } ) ); | ||
| } ); | ||
|
|
||
| const { container } = render( <FullscreenModeClose /> ); | ||
| const siteIcon = container.querySelector( | ||
| '.edit-site-fullscreen-mode-close_site-icon' | ||
| ); | ||
|
|
||
| expect( siteIcon ).toBeTruthy(); | ||
| } ); | ||
|
|
||
| it( 'should display a default site icon if no user uploaded site icon exists', () => { | ||
| useSelect.mockImplementation( ( cb ) => { | ||
| return cb( () => ( { | ||
| isResolving: () => false, | ||
| isFeatureActive: () => true, | ||
| getEntityRecord: () => ( { | ||
| site_icon_url: '', | ||
| } ), | ||
| } ) ); | ||
| } ); | ||
|
|
||
| const { container } = render( <FullscreenModeClose /> ); | ||
| const siteIcon = container.querySelector( | ||
| '.edit-site-fullscreen-mode-close_site-icon' | ||
| ); | ||
| const defaultIcon = container.querySelector( 'svg' ); | ||
|
|
||
| expect( siteIcon ).toBeFalsy(); | ||
| expect( defaultIcon ).toBeTruthy(); | ||
| } ); | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ticket has been rejected on Core, should we remove that code from here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads up Riad 👍 This is on my radar -- will respond when I get the chance tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been quite a while since I've looked at this code, but yeah removing it makes sense to me, especially if, according to Timothy, the index endpoint doesn't even support
contextquery params.I'll spin up a draft to remove this code soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an attempt at making the entity request urls more specific to each entity, and prevents the need for preloading
/?context=edit: #30482