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

Update the edit site save modal UI #20608

Merged
merged 4 commits into from
Mar 5, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
15 changes: 0 additions & 15 deletions docs/designers-developers/developers/data/data-core.md
Original file line number Diff line number Diff line change
Expand Up @@ -153,21 +153,6 @@ _Returns_

- `?Object`: Record.

<a name="getEntityRecordChangesByRecord" href="#getEntityRecordChangesByRecord">#</a> **getEntityRecordChangesByRecord**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This selector was very hard to work with and I checked wpdirectory.net it's not something that is used by plugins and is unlikely to be used before FSE.


Returns a map of objects with each edited
raw entity record and its corresponding edits.

The map is keyed by entity `kind => name => key => { rawRecord, edits }`.

_Parameters_

- _state_ `Object`: State tree.

_Returns_

- (unknown type): The map of edited records with their edits.

<a name="getEntityRecordEdits" href="#getEntityRecordEdits">#</a> **getEntityRecordEdits**

Returns the specified entity record's edits.
Expand Down
9 changes: 5 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/components/src/base-control/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@
}

.components-base-control + .components-base-control {
margin-bottom: $grid-unit-20;
margin-top: $grid-unit-20;
Copy link
Contributor

Choose a reason for hiding this comment

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

This caused the following regression:

Screenshot 2020-03-11 at 10 30 55

What effort did it solve to make the margin a top margin rather than a bottom margin? Because I'm looking at fixing this in a separate PR and would like to avoid re-introducing the same issue you solved in making this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well this rule is meant to style consecutive controls right to add space between them. and in general consecutive controls are one under the other (unless some flex positioning in the parent is involved) which means the margin should be top since the selector targets the second element.

This was causing weird margins between the multiple entities in the popover.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well this rule is meant to style consecutive controls right to add space between them.

I don't think that was the intent of the rule, but if it was, then I agree the margin should be at the top. I'll look for a fix that doesn't style multiple consecutive base controls in this way.

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 can't think of any other reason why this rule could exist, and also I wonder if it's me that introduce this rule from the first release of BaseControl :P .

Copy link
Contributor

Choose a reason for hiding this comment

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

In #20782 I removed it, an so far not seeing any issues.

As far as I can tell, the rule was introduced here: https://github.com/WordPress/gutenberg/pull/13181/files#diff-7f4a5101433bca0f9e460c45196b772fR24

From the conversation, it's not immediately clear why that particular rule was necessary. But given additional sidebar polish that is meant to happen soon, I would expect these spacing rules to be revisited regardless.

}
2 changes: 1 addition & 1 deletion packages/components/src/color-picker/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@
margin-right: 4px;

.components-base-control + .components-base-control {
margin-bottom: 0;
margin-top: 0;
}

.components-base-control__field {
Expand Down
15 changes: 0 additions & 15 deletions packages/core-data/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -366,21 +366,6 @@ _Returns_

- `?Object`: Record.

<a name="getEntityRecordChangesByRecord" href="#getEntityRecordChangesByRecord">#</a> **getEntityRecordChangesByRecord**

Returns a map of objects with each edited
raw entity record and its corresponding edits.

The map is keyed by entity `kind => name => key => { rawRecord, edits }`.

_Parameters_

- _state_ `Object`: State tree.

_Returns_

- (unknown type): The map of edited records with their edits.

<a name="getEntityRecordEdits" href="#getEntityRecordEdits">#</a> **getEntityRecordEdits**

Returns the specified entity record's edits.
Expand Down
1 change: 1 addition & 0 deletions packages/core-data/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"@wordpress/data": "file:../data",
"@wordpress/deprecated": "file:../deprecated",
"@wordpress/element": "file:../element",
"@wordpress/i18n": "file:../i18n",
"@wordpress/is-shallow-equal": "file:../is-shallow-equal",
"@wordpress/url": "file:../url",
"equivalent-key-map": "^0.2.2",
Expand Down
41 changes: 37 additions & 4 deletions packages/core-data/src/entities.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
/**
* External dependencies
*/
import { upperFirst, camelCase, map, find } from 'lodash';
import { upperFirst, camelCase, map, find, get, startCase } from 'lodash';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
Expand All @@ -12,29 +17,49 @@ import { apiFetch, select } from './controls';
export const DEFAULT_ENTITY_KEY = 'id';

export const defaultEntities = [
{ name: 'site', kind: 'root', baseURL: '/wp/v2/settings' },
{ name: 'postType', kind: 'root', key: 'slug', baseURL: '/wp/v2/types' },
{
label: __( 'Site' ),
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 had to introduce this "label" property and also a dependency towards i18n package.

name: 'site',
kind: 'root',
baseURL: '/wp/v2/settings',
},
{
label: __( 'Post Type' ),
name: 'postType',
kind: 'root',
key: 'slug',
baseURL: '/wp/v2/types',
},
{
name: 'media',
kind: 'root',
baseURL: '/wp/v2/media',
plural: 'mediaItems',
label: __( 'Media' ),
},
{
name: 'taxonomy',
kind: 'root',
key: 'slug',
baseURL: '/wp/v2/taxonomies',
plural: 'taxonomies',
label: __( 'Taxonomy' ),
},
{
name: 'widgetArea',
kind: 'root',
baseURL: '/__experimental/widget-areas',
plural: 'widgetAreas',
transientEdits: { blocks: true },
label: __( 'Widget area' ),
},
{
label: __( 'User' ),
name: 'user',
kind: 'root',
baseURL: '/wp/v2/users',
plural: 'users',
},
{ name: 'user', kind: 'root', baseURL: '/wp/v2/users', plural: 'users' },
];

export const kinds = [
Expand All @@ -54,12 +79,19 @@ function* loadPostTypeEntities() {
kind: 'postType',
baseURL: '/wp/v2/' + postType.rest_base,
name,
label: postType.labels.singular_name,
transientEdits: {
blocks: true,
selectionStart: true,
selectionEnd: true,
},
mergedEdits: { meta: true },
getTitle( record ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is supposed to be a generic way to get a "title"/"caption"/"label" of any Entity Record. Notice that it still needs some specific behavior.

if ( name === 'wp_template_part' || name === 'wp_template' ) {
return startCase( record.slug );
}
return get( record, [ 'title', 'rendered' ], record.id );
},
};
} );
}
Expand All @@ -78,6 +110,7 @@ function* loadTaxonomyEntities() {
kind: 'taxonomy',
baseURL: '/wp/v2/' + taxonomy.rest_base,
name,
label: taxonomy.labels.singular_name,
};
} );
}
Expand Down
71 changes: 35 additions & 36 deletions packages/core-data/src/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import deprecated from '@wordpress/deprecated';
*/
import { REDUCER_KEY } from './name';
import { getQueriedItems } from './queried-data';
import { DEFAULT_ENTITY_KEY } from './entities';

/**
* Returns true if a request is in progress for embed preview data, or false
Expand Down Expand Up @@ -193,56 +194,54 @@ export function getEntityRecords( state, kind, name, query ) {
}

/**
* Returns a map of objects with each edited
* raw entity record and its corresponding edits.
*
* The map is keyed by entity `kind => name => key => { rawRecord, edits }`.
* Returns the list of dirty entity records.
*
* @param {Object} state State tree.
*
* @return {{ [kind: string]: { [name: string]: { [key: string]: { rawRecord: Object<string,*>, edits: Object<string,*> } } } }} The map of edited records with their edits.
* @return {[{ title: string, key: string, name: string, kind: string }]} The list of updated records
*/
export const getEntityRecordChangesByRecord = createSelector(
export const __experimentalGetDirtyEntityRecords = createSelector(
( state ) => {
const {
entities: { data },
} = state;
return Object.keys( data ).reduce( ( acc, kind ) => {
const dirtyRecords = [];
Object.keys( data ).forEach( ( kind ) => {
Object.keys( data[ kind ] ).forEach( ( name ) => {
const editsKeys = Object.keys(
const primaryKeys = Object.keys(
data[ kind ][ name ].edits
).filter( ( editsKey ) =>
hasEditsForEntityRecord( state, kind, name, editsKey )
).filter( ( primaryKey ) =>
hasEditsForEntityRecord( state, kind, name, primaryKey )
);
if ( editsKeys.length ) {
if ( ! acc[ kind ] ) {
acc[ kind ] = {};
}
if ( ! acc[ kind ][ name ] ) {
acc[ kind ][ name ] = {};
}
editsKeys.forEach(
( editsKey ) =>
( acc[ kind ][ name ][ editsKey ] = {
rawRecord: getRawEntityRecord(
state,
kind,
name,
editsKey
),
edits: getEntityRecordNonTransientEdits(
state,
kind,
name,
editsKey
),
} )
);

if ( primaryKeys.length ) {
const entity = getEntity( state, kind, name );
primaryKeys.forEach( ( primaryKey ) => {
const entityRecord = getEntityRecord(
state,
kind,
name,
primaryKey
);
dirtyRecords.push( {
// We avoid using primaryKey because it's transformed into a string
// when it's used as an object key.
key:
entityRecord[
entity.key || DEFAULT_ENTITY_KEY
],
title: ! entity.getTitle
? ''
: entity.getTitle( entityRecord ),
name,
kind,
} );
} );
}
} );
} );

return acc;
}, {} );
return dirtyRecords;
},
( state ) => [ state.entities.data ]
);
Expand Down
3 changes: 2 additions & 1 deletion packages/edit-site/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@
"@wordpress/notices": "file:../notices",
"@wordpress/url": "file:../url",
"file-saver": "^2.0.2",
"jszip": "^3.2.2"
"jszip": "^3.2.2",
"lodash": "^4.17.15"
},
"publishConfig": {
"access": "public"
Expand Down
35 changes: 13 additions & 22 deletions packages/edit-site/src/components/save-button/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import { some } from 'lodash';

/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -33,29 +38,15 @@ export default function SaveButton() {
}, [ slug ] );

const { isDirty, isSaving } = useSelect( ( select ) => {
const { getEntityRecordChangesByRecord, isSavingEntityRecord } = select(
'core'
);
const entityRecordChangesByRecord = getEntityRecordChangesByRecord();
const changedKinds = Object.keys( entityRecordChangesByRecord );
const {
__experimentalGetDirtyEntityRecords,
isSavingEntityRecord,
} = select( 'core' );
const dirtyEntityRecords = __experimentalGetDirtyEntityRecords();
return {
isDirty: changedKinds.length > 0,
isSaving: changedKinds.some( ( changedKind ) =>
Object.keys(
entityRecordChangesByRecord[ changedKind ]
).some( ( changedName ) =>
Object.keys(
entityRecordChangesByRecord[ changedKind ][
changedName
]
).some( ( changedKey ) =>
isSavingEntityRecord(
changedKind,
changedName,
changedKey
)
)
)
isDirty: dirtyEntityRecords.length > 0,
isSaving: some( dirtyEntityRecords, ( record ) =>
isSavingEntityRecord( record.kind, record.name, record.key )
),
};
} );
Expand Down
1 change: 0 additions & 1 deletion packages/editor/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
"@wordpress/viewport": "file:../viewport",
"@wordpress/wordcount": "file:../wordcount",
"classnames": "^2.2.5",
"equivalent-key-map": "^0.2.2",
"lodash": "^4.17.15",
"memize": "^1.0.5",
"react-autosize-textarea": "^3.0.2",
Expand Down
Loading