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

Jump to bottom

GlobalStyles: Save and Delete JS changes #47076

Open
wants to merge 6 commits into
base: rpp-php-changes
Choose a base branch
from
+778 −55
Open

GlobalStyles: Save and Delete JS changes #47076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
19a9cf6
GlobalStyles: Save and Delete JS changes
ribaricplusplus Jan 11, 2023
5a122e2
Remove user variations receiving functionality
ribaricplusplus Jan 11, 2023
4bc24fa
Optimization
ribaricplusplus Jan 11, 2023
29b971f
Fixes
ribaricplusplus Jan 11, 2023
dc94b58
Fixes
ribaricplusplus Jan 11, 2023
4a6b7d7
Merge branch 'rpp-php-changes' into rpp-js-changes
ribaricplusplus Jan 21, 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
37 changes: 37 additions & 0 deletions packages/core-data/src/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,3 +834,40 @@ export function receiveAutosaves( postId, autosaves ) {
autosaves: Array.isArray( autosaves ) ? autosaves : [ autosaves ],
};
}

/**
* Discards changes in the specified record.
*
* @param {string} kind Entity kind.
* @param {string} name Entity name.
* @param {*} recordId Record ID.
*
*/
export const __experimentalDiscardRecordChanges =
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this, a sort of undo? Should we just refetch the original instead?

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 a sort of undo yes. We cannot just refetch the original because when you click the "save" button, it will still show that the record has changes. Suppose you have a record which is in state 1. Then user does stuff and changes it to state 2. But then the user does something which should undo the changes. If we refetch the original so the record goes back to state 1, in the Redux store that tracks changes it will see the record as going State 1 -> State 2 -> State 1 and the record will be considered "dirty" even though it's not dirty.

Copy link
Contributor

Choose a reason for hiding this comment

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

State 1 -> State 2 -> State 1 and the record will be considered "dirty" even though it's not dirty.

I'm pretty sure we already have logic to handle this in core-data. you can confirm that by opening the post editor, writing a title, saving, adding a letter and removing a letter, you'll see that the post is not dirty.

( kind, name, recordId ) =>
( { dispatch, select } ) => {
const edits = select.getEntityRecordEdits( kind, name, recordId );

if ( ! edits ) {
return;
}

const clearedEdits = Object.keys( edits ).reduce( ( acc, key ) => {
return {
...acc,
[ key ]: undefined,
};
}, {} );

dispatch( {
type: 'EDIT_ENTITY_RECORD',
kind,
name,
recordId,
edits: clearedEdits,
transientEdits: clearedEdits,
meta: {
isUndo: false,
},
} );
};
8 changes: 4 additions & 4 deletions packages/core-data/src/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export interface State {
embedPreviews: Record< string, { html: string } >;
entities: EntitiesState;
themeBaseGlobalStyles: Record< string, Object >;
themeGlobalStyleVariations: Record< string, string >;
themeGlobalStyleVariations: Record< string, Object[] >;
undo: UndoState;
users: UserState;
}
Expand Down Expand Up @@ -1238,15 +1238,15 @@ export function __experimentalGetCurrentThemeBaseGlobalStyles(
}

/**
* Return the ID of the current global styles object.
* Return global styles variations.
*
* @param state Data state.
*
* @return The current global styles ID.
* @return Global styles variations
*/
export function __experimentalGetCurrentThemeGlobalStylesVariations(
state: State
): string | null {
): Object[] | null {
const currentTheme = getCurrentTheme( state );
if ( ! currentTheme ) {
return null;
Expand Down
32 changes: 32 additions & 0 deletions packages/e2e-tests/plugins/global-styles.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php
/**
* Plugin Name: Gutenberg Test Plugin, Global Styles
* Plugin URI: https://github.com/WordPress/gutenberg
* Author: Gutenberg Team
*
* @package gutenberg-test-plugin-global-styles
*/

/**
* Add REST endpoint.
*/
function gutenberg_add_delete_all_global_styles_endpoint() {
register_rest_route(
'wp/v2',
'/delete-all-global-styles',
array(
array(
'methods' => WP_REST_Server::DELETABLE,
'callback' => function() {
global $wpdb;
$gs_id = WP_Theme_JSON_Resolver_Gutenberg::get_user_global_styles_post_id();
// phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared
$wpdb->get_results( "DELETE FROM {$wpdb->posts} WHERE post_type = 'wp_global_styles' AND id != {$gs_id}" );
return rest_ensure_response( array( 'deleted' => true ) );
},
'permission_callback' => '__return_true',
),
)
);
}
add_action( 'rest_api_init', 'gutenberg_add_delete_all_global_styles_endpoint' );
154 changes: 138 additions & 16 deletions packages/edit-site/src/components/global-styles/global-styles-provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { mergeWith, isEmpty, mapValues } from 'lodash';
/**
* WordPress dependencies
*/
import { useMemo, useCallback } from '@wordpress/element';
import { useMemo, useCallback, useState } from '@wordpress/element';
import { useSelect, useDispatch } from '@wordpress/data';
import { store as coreStore } from '@wordpress/core-data';
import { experiments as blockEditorExperiments } from '@wordpress/block-editor';
Expand All @@ -19,6 +19,8 @@ import { unlock } from '../../experiments';

const { GlobalStylesContext } = unlock( blockEditorExperiments );

/* eslint-disable dot-notation, camelcase */
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a variable called associated_style_id that gets used and it is in snake case notation because it comes from PHP and the JS linter is upset about it.


function mergeTreesCustomizer( _, srcValue ) {
// We only pass as arrays the presets,
// in which case we want the new array of values
Expand Down Expand Up @@ -49,10 +51,15 @@ const cleanEmptyObject = ( object ) => {
};

function useGlobalStylesUserConfig() {
const { globalStylesId, isReady, settings, styles } = useSelect(
const { hasFinishedResolution } = useSelect( coreStore );
const { editEntityRecord, __experimentalDiscardRecordChanges } =
useDispatch( coreStore );

const [ isReady, setIsReady ] = useState( false );

const { globalStylesId, settings, styles, associated_style_id } = useSelect(
( select ) => {
const { getEditedEntityRecord, hasFinishedResolution } =
select( coreStore );
const { getEditedEntityRecord } = select( coreStore );
const _globalStylesId =
select( coreStore ).__experimentalGetCurrentGlobalStylesId();
const record = _globalStylesId
Expand All @@ -62,40 +69,74 @@ function useGlobalStylesUserConfig() {
_globalStylesId
)
: undefined;
const _associatedStyleId = record
Copy link
Contributor

Choose a reason for hiding this comment

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

As I commented in the PHP PR as well, we need to explain in the comments what associated styles are.

? record[ 'associated_style_id' ]
: undefined;
if ( _associatedStyleId ) {
getEditedEntityRecord(
'root',
'globalStyles',
_associatedStyleId
);
}

let hasResolved = false;
if (
! isReady &&
hasFinishedResolution(
'__experimentalGetCurrentGlobalStylesId'
)
) {
hasResolved = _globalStylesId
? hasFinishedResolution( 'getEditedEntityRecord', [
hasResolved = ( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what isReady does that hasResolved doesn't? Is it a sort of Promise.all()?

if ( ! _globalStylesId ) {
return true;
}

const userStyleFinishedResolution = hasFinishedResolution(
'getEditedEntityRecord',
[ 'root', 'globalStyles', _globalStylesId ]
);

if ( ! _associatedStyleId ) {
return userStyleFinishedResolution;
}

const associatedStyleFinishedResolution =
hasFinishedResolution( 'getEditedEntityRecord', [
'root',
'globalStyles',
_globalStylesId,
] )
: true;
_associatedStyleId,
] );

return (
userStyleFinishedResolution &&
associatedStyleFinishedResolution
);
} )();

if ( hasResolved && ! isReady ) {
setIsReady( true );
}
}

return {
globalStylesId: _globalStylesId,
isReady: hasResolved,
settings: record?.settings,
styles: record?.styles,
associated_style_id: _associatedStyleId,
};
},
[]
);

const { getEditedEntityRecord } = useSelect( coreStore );
const { editEntityRecord } = useDispatch( coreStore );
const config = useMemo( () => {
return {
settings: settings ?? {},
styles: styles ?? {},
associated_style_id: associated_style_id ?? null,
};
}, [ settings, styles ] );
}, [ settings, styles, associated_style_id ] );

const setConfig = useCallback(
( callback, options = {} ) => {
Expand All @@ -107,18 +148,97 @@ function useGlobalStylesUserConfig() {
const currentConfig = {
styles: record?.styles ?? {},
settings: record?.settings ?? {},
associated_style_id: record?.associated_style_id ?? 0,
};
const updatedConfig = callback( currentConfig );
const updatedRecord = {
styles: cleanEmptyObject( updatedConfig.styles ) || {},
settings: cleanEmptyObject( updatedConfig.settings ) || {},
associated_style_id:
updatedConfig[ 'associated_style_id' ] || 0,
};

let associatedStyleIdChanged = false;

if (
currentConfig[ 'associated_style_id' ] !==
updatedRecord[ 'associated_style_id' ]
) {
associatedStyleIdChanged = true;
__experimentalDiscardRecordChanges(
'root',
'globalStyles',
currentConfig[ 'associated_style_id' ]
);
}

editEntityRecord(
'root',
'globalStyles',
globalStylesId,
{
styles: cleanEmptyObject( updatedConfig.styles ) || {},
settings: cleanEmptyObject( updatedConfig.settings ) || {},
},
updatedRecord,
options
);

// Also add changes that were made to the user record to the associated record.
if (
! associatedStyleIdChanged &&
updatedRecord[ 'associated_style_id' ]
) {
if (
( ! hasFinishedResolution( 'getEditedEntityRecord' ),
[
'root',
'globalStyles',
updatedRecord[ 'associated_style_id' ],
] )
) {
let numTries = 0;
const MAX_NUM_TRIES = 30;
const intervalId = setInterval( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to retry 30 times?

++numTries;

if ( numTries > MAX_NUM_TRIES ) {
clearInterval( intervalId );
throw new Error(
'Failed editing associated global style entity record.'
);
}

if (
( hasFinishedResolution( 'getEditedEntityRecord' ),
[
'root',
'globalStyles',
updatedRecord[ 'associated_style_id' ],
] )
) {
editEntityRecord(
'root',
'globalStyles',
updatedRecord[ 'associated_style_id' ],
{
settings: updatedRecord.settings,
styles: updatedRecord.styles,
},
options
);
clearInterval( intervalId );
}
}, 500 );
} else {
editEntityRecord(
'root',
'globalStyles',
updatedRecord[ 'associated_style_id' ],
{
settings: updatedRecord.settings,
styles: updatedRecord.styles,
},
options
);
}
}
},
[ globalStylesId ]
);
Expand Down Expand Up @@ -166,6 +286,8 @@ function useGlobalStylesContext() {
return context;
}

/* eslint-enable dot-notation, camelcase */

export function GlobalStylesProvider( { children } ) {
const context = useGlobalStylesContext();
if ( ! context.isReady ) {
Expand Down
Loading