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

Global styles revisions: display text if no revisions are found #52865

Merged
merged 4 commits into from
Jul 24, 2023
Merged

Global styles revisions: display text if no revisions are found #52865

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e9d2277
If somehow a user lands on the revisions panel when there are no revi…
ramonjd Jul 24, 2023
104b83c
Updated unit tests to reflect resolver logic changes
ramonjd Jul 24, 2023
2ddedb2
Use existing string
ramonjd Jul 24, 2023
b16b034
Only open edit view when testing the revisions panel itself
ramonjd Jul 24, 2023
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/data/data-core.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ _Parameters_

_Returns_

- `Object | null`: The current global styles.
- `Array< object > | null`: The current global styles.

### getCurrentUser

Expand Down
2 changes: 1 addition & 1 deletion packages/core-data/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ _Parameters_

_Returns_

- `Object | null`: The current global styles.
- `Array< object > | null`: The current global styles.

### getCurrentUser

Expand Down
2 changes: 1 addition & 1 deletion packages/core-data/src/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,7 +1266,7 @@ export function getBlockPatternCategories( state: State ): Array< any > {
*/
export function getCurrentThemeGlobalStylesRevisions(
state: State
): Object | null {
): Array< object > | null {
const currentGlobalStylesId =
__experimentalGetCurrentGlobalStylesId( state );

Expand Down
132 changes: 73 additions & 59 deletions packages/edit-site/src/components/global-styles/screen-revisions/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
__experimentalUseNavigator as useNavigator,
__experimentalConfirmDialog as ConfirmDialog,
Spinner,
__experimentalSpacer as Spacer,
} from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';
import { useContext, useState, useEffect } from '@wordpress/element';
Expand Down Expand Up @@ -89,6 +90,7 @@ function ScreenRevisions() {
const isLoadButtonEnabled =
!! globalStylesRevision?.id &&
! areGlobalStyleConfigsEqual( globalStylesRevision, userConfig );
const shouldShowRevisions = ! isLoading && revisions.length;

return (
<>
Expand All @@ -101,68 +103,80 @@ function ScreenRevisions() {
{ isLoading && (
<Spinner className="edit-site-global-styles-screen-revisions__loading" />
) }
{ ! isLoading && (
<Revisions
blocks={ blocks }
userConfig={ globalStylesRevision }
onClose={ onCloseRevisions }
/>
) }
<div className="edit-site-global-styles-screen-revisions">
<RevisionsButtons
onChange={ selectRevision }
selectedRevisionId={ selectedRevisionId }
userRevisions={ revisions }
/>
{ isLoadButtonEnabled && (
<SidebarFixedBottom>
<Button
variant="primary"
className="edit-site-global-styles-screen-revisions__button"
disabled={
! globalStylesRevision?.id ||
globalStylesRevision?.id === 'unsaved'
}
onClick={ () => {
if ( hasUnsavedChanges ) {
setIsLoadingRevisionWithUnsavedChanges(
true
);
} else {
restoreRevision( globalStylesRevision );
}
} }
>
{ __( 'Apply' ) }
</Button>
</SidebarFixedBottom>
) }
</div>
{ isLoadingRevisionWithUnsavedChanges && (
<ConfirmDialog
title={ __(
'Loading this revision will discard all unsaved changes.'
) }
isOpen={ isLoadingRevisionWithUnsavedChanges }
confirmButtonText={ __( ' Discard unsaved changes' ) }
onConfirm={ () => restoreRevision( globalStylesRevision ) }
onCancel={ () =>
setIsLoadingRevisionWithUnsavedChanges( false )
}
>
<>
<h2>
{ __(
{ shouldShowRevisions ? (
<>
<Revisions
blocks={ blocks }
userConfig={ globalStylesRevision }
onClose={ onCloseRevisions }
/>
<div className="edit-site-global-styles-screen-revisions">
<RevisionsButtons
onChange={ selectRevision }
selectedRevisionId={ selectedRevisionId }
userRevisions={ revisions }
/>
{ isLoadButtonEnabled && (
<SidebarFixedBottom>
<Button
variant="primary"
className="edit-site-global-styles-screen-revisions__button"
disabled={
! globalStylesRevision?.id ||
globalStylesRevision?.id === 'unsaved'
}
onClick={ () => {
if ( hasUnsavedChanges ) {
setIsLoadingRevisionWithUnsavedChanges(
true
);
} else {
restoreRevision(
globalStylesRevision
);
}
} }
>
{ __( 'Apply' ) }
</Button>
</SidebarFixedBottom>
) }
</div>
{ isLoadingRevisionWithUnsavedChanges && (
<ConfirmDialog
title={ __(
'Loading this revision will discard all unsaved changes.'
) }
</h2>
<p>
{ __(
'Do you want to replace your unsaved changes in the editor?'
isOpen={ isLoadingRevisionWithUnsavedChanges }
confirmButtonText={ __(
' Discard unsaved changes'
) }
</p>
</>
</ConfirmDialog>
onConfirm={ () =>
restoreRevision( globalStylesRevision )
}
onCancel={ () =>
setIsLoadingRevisionWithUnsavedChanges( false )
}
>
<>
<h2>
{ __(
'Loading this revision will discard all unsaved changes.'
) }
</h2>
<p>
{ __(
'Do you want to replace your unsaved changes in the editor?'
) }
</p>
</>
</ConfirmDialog>
) }
</>
) : (
<Spacer marginX={ 4 } data-testid="global-styles-no-revisions">
{ __( 'There are currently no style revisions.' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a new string? Since we're in the RC period, I was wondering if it's an issue that it's being added. If so, could we temporarily reuse an existing string from somewhere? No results found. isn't very friendly, but appears to already exist, in case that's useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be an issue, you're right. Thanks for flagging and suggesting a new string. I YOLO'ed the RC label. I'll try to find an existing string.

</Spacer>
) }
</>
);
Expand Down
113 changes: 64 additions & 49 deletions ...es/edit-site/src/components/global-styles/screen-revisions/use-global-styles-revisions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,34 +21,40 @@ const EMPTY_ARRAY = [];
const { GlobalStylesContext } = unlock( blockEditorPrivateApis );
export default function useGlobalStylesRevisions() {
const { user: userConfig } = useContext( GlobalStylesContext );
const { authors, currentUser, isDirty, revisions } = useSelect(
( select ) => {
const {
__experimentalGetDirtyEntityRecords,
getCurrentUser,
getUsers,
getCurrentThemeGlobalStylesRevisions,
} = select( coreStore );
const dirtyEntityRecords = __experimentalGetDirtyEntityRecords();
const _currentUser = getCurrentUser();
const _isDirty = dirtyEntityRecords.length > 0;
const globalStylesRevisions =
getCurrentThemeGlobalStylesRevisions() || EMPTY_ARRAY;
const _authors =
getUsers( SITE_EDITOR_AUTHORS_QUERY ) || EMPTY_ARRAY;
const {
authors,
currentUser,
isDirty,
revisions,
isLoadingGlobalStylesRevisions,
} = useSelect( ( select ) => {
const {
__experimentalGetDirtyEntityRecords,
getCurrentUser,
getUsers,
getCurrentThemeGlobalStylesRevisions,
isResolving,
} = select( coreStore );
const dirtyEntityRecords = __experimentalGetDirtyEntityRecords();
const _currentUser = getCurrentUser();
const _isDirty = dirtyEntityRecords.length > 0;
const globalStylesRevisions =
getCurrentThemeGlobalStylesRevisions() || EMPTY_ARRAY;
const _authors = getUsers( SITE_EDITOR_AUTHORS_QUERY ) || EMPTY_ARRAY;

return {
authors: _authors,
currentUser: _currentUser,
isDirty: _isDirty,
revisions: globalStylesRevisions,
};
},
[]
);
return {
authors: _authors,
currentUser: _currentUser,
isDirty: _isDirty,
revisions: globalStylesRevisions,
isLoadingGlobalStylesRevisions: isResolving(
'getCurrentThemeGlobalStylesRevisions'
),
};
}, [] );
return useMemo( () => {
let _modifiedRevisions = [];
if ( ! authors.length || ! revisions.length ) {
if ( ! authors.length || isLoadingGlobalStylesRevisions ) {
return {
revisions: _modifiedRevisions,
hasUnsavedChanges: isDirty,
Expand All @@ -66,37 +72,46 @@ export default function useGlobalStylesRevisions() {
};
} );

// Flags the most current saved revision.
if ( _modifiedRevisions[ 0 ].id !== 'unsaved' ) {
_modifiedRevisions[ 0 ].isLatest = true;
}
if ( _modifiedRevisions.length ) {
// Flags the most current saved revision.
if ( _modifiedRevisions[ 0 ].id !== 'unsaved' ) {
_modifiedRevisions[ 0 ].isLatest = true;
}

// Adds an item for unsaved changes.
if (
isDirty &&
userConfig &&
Object.keys( userConfig ).length > 0 &&
currentUser
) {
const unsavedRevision = {
id: 'unsaved',
styles: userConfig?.styles,
settings: userConfig?.settings,
behaviors: userConfig?.behaviors,
author: {
name: currentUser?.name,
avatar_urls: currentUser?.avatar_urls,
},
modified: new Date(),
};
// Adds an item for unsaved changes.
if (
isDirty &&
userConfig &&
Object.keys( userConfig ).length > 0 &&
currentUser
) {
const unsavedRevision = {
id: 'unsaved',
styles: userConfig?.styles,
settings: userConfig?.settings,
behaviors: userConfig?.behaviors,
author: {
name: currentUser?.name,
avatar_urls: currentUser?.avatar_urls,
},
modified: new Date(),
};

_modifiedRevisions.unshift( unsavedRevision );
_modifiedRevisions.unshift( unsavedRevision );
}
}

return {
revisions: _modifiedRevisions,
hasUnsavedChanges: isDirty,
isLoading: false,
};
}, [ isDirty, revisions, currentUser, authors, userConfig ] );
}, [
isDirty,
revisions,
currentUser,
authors,
userConfig,
isLoadingGlobalStylesRevisions,
] );
}
13 changes: 13 additions & 0 deletions test/e2e/specs/site-editor/user-global-styles-revisions.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@ test.describe( 'Global styles revisions', () => {
await editor.canvas.click( 'body' );
} );

test( 'should display no revisions message if landing via command center', async ( {
page,
} ) => {
await page.keyboard.press( 'Meta+k' );
Copy link
Member

Choose a reason for hiding this comment

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

Based on failure traces, the command doesn't open the Command Palette. You can use https://trace.playwright.dev/ to check the Playwright - https://trace.playwright.dev/.

The reasons could be that there is no focus on the editor's body or keyboard shortcuts must be corrected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, interesting. Thanks for the tips. It's passing locally, so I'll try a few things and push them 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it helps, but it looks like the Open the command palette and navigate to the page create page test moves focus to the command palette button before pressing Meta+k, so something like that might make it more reliable? E.g.

gutenberg/test/e2e/specs/site-editor/command-center.spec.js

Lines 23 to 25 in a3d81f6

await page
.getByRole( 'button', { name: 'Open command palette' } )
.focus();

Copy link
Member Author

Choose a reason for hiding this comment

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

but it looks like the Open the command palette and navigate to the page create page test moves focus to the command palette button before pressing Meta+k, so something like that might make it more reliable?

Thanks @andrewserong

That was actually the first thing I tried (copy paste 😄 ) but it wasn't passing locally with that line.

Maybe I'll add it and YOLO it up to CI

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because that button only exists in view mode. I'm opening the canvas in edit mode. 👍🏻

Copy link
Member

@Mamaduka Mamaduka Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We could just click the button if we're already locating it 😄 But as long as it works, I'm fine with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could just click the button if we're already locating it 😄 But as long as it works, I'm fine with it.

I ❤️ copy 🍝

Copy link
Member

Choose a reason for hiding this comment

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

🍝 goes well with e2e tests 😁

await page.keyboard.type( 'styles revisions' );
await page
.getByRole( 'option', { name: 'Open styles revisions' } )
.click();
await expect(
page.getByTestId( 'global-styles-no-revisions' )
).toHaveText( 'There are currently no style revisions.' );
} );

test( 'should display revisions UI when there is more than 1 revision', async ( {
page,
editor,
Expand Down