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: update return values from getGlobalStylesChanges() #58707

Merged
merged 2 commits into from
Feb 6, 2024
Merged

Global styles: update return values from getGlobalStylesChanges() #58707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d33ad4
Leave it up to the consuming component as to how to present the chang…
ramonjd Feb 6, 2024
7a79094
Period.
ramonjd Feb 6, 2024
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
10 changes: 2 additions & 8 deletions packages/block-editor/src/components/global-styles/get-global-styles-changes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,7 @@ function getTranslation( key ) {

if ( keyArray?.[ 0 ] === 'blocks' ) {
const blockName = getBlockNames()?.[ keyArray[ 1 ] ];
return blockName
? sprintf(
// translators: %s: block name.
__( '%s block' ),
blockName
)
: keyArray[ 1 ];
return blockName || keyArray[ 1 ];
}

if ( keyArray?.[ 0 ] === 'elements' ) {
Expand Down Expand Up @@ -200,7 +194,7 @@ export default function getGlobalStylesChanges( next, previous, options = {} ) {
const deleteCount = changesLength - maxResults;
const andMoreText = sprintf(
// translators: %d: number of global styles changes that are not displayed in the UI.
_n( '…and %d more change.', '…and %d more changes.', deleteCount ),
_n( '…and %d more change', '…and %d more changes', deleteCount ),
deleteCount
);
changes.splice( maxResults, deleteCount, andMoreText );
Expand Down
6 changes: 3 additions & 3 deletions packages/block-editor/src/components/global-styles/test/get-global-styles-changes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ describe( 'getGlobalStylesChanges', () => {
expect( resultA ).toEqual( [
'Colors',
'Typography',
'Test pumpkin flowers block',
'Test pumpkin flowers',
'H3 element',
'Caption element',
'H6 element',
Expand All @@ -191,8 +191,8 @@ describe( 'getGlobalStylesChanges', () => {
expect( resultA ).toEqual( [
'Colors',
'Typography',
'Test pumpkin flowers block',
'…and 5 more changes.',
'Test pumpkin flowers',
'…and 5 more changes',
] );
} );

Expand Down
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/global-styles/screen-revisions/revisions-buttons.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ function ChangesSummary( { revision, previousRevision } ) {
data-testid="global-styles-revision-changes"
className="edit-site-global-styles-screen-revisions__changes"
>
{ changes.join( ', ' ) }
{ changes.join( ', ' ) }.
Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty crude but it was the simplest approach after having tried the following:

  • passing a "suffix" option value of "." to getGlobalStylesChanges(). Rejected because the function returns an array of strings and requires appending the suffix to the last item. Too weird.
  • Changing the return value of getGlobalStylesChanges() to be a joined string. Rejected because it removes all the flexibility of a string[] return value.
  • Creating a new function to return changes.join( ', ' ) }.. Rejected, or at least postponed, until we need it. We're only using getGlobalStylesChanges() in two places.

In summary, I'm of the opinion that the component should decide how to present the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're already using , to join the changes together, I think appending a . character here is likely fine!

</span>
);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/editor/src/components/entities-saved-states/entity-type-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ function GlobalStylesDescription( { record } ) {
<h3 className="entities-saved-states__description-heading">
{ __( 'Changes made to:' ) }
</h3>
<PanelRow>{ globalStylesChanges.join( ', ' ) }</PanelRow>
<PanelRow>{ globalStylesChanges.join( ', ' ) }.</PanelRow>
</>
) : null;
}
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -57,7 +57,7 @@ test.describe( 'Style Revisions', () => {
// Shows changes made in the revision.
await expect(
page.getByTestId( 'global-styles-revision-changes' )
).toHaveText( 'Colors' );
).toHaveText( 'Colors.' );

// There should be 2 revisions not including the reset to theme defaults button.
await expect( revisionButtons ).toHaveCount(
Expand Down
Loading