Update the edit site save modal UI #20608
Conversation
youknowriad
added
Needs Design Feedback
youknowriad
requested review from
ajitbohra,
chrisvanpatten,
jorgefilipecosta,
mkaz and
nerrad
as code owners
| @@ -153,21 +153,6 @@ _Returns_ | |||
|
|
|||
| - `?Object`: Record. | |||
|
|
|||
| <a name="getEntityRecordChangesByRecord" href="#getEntityRecordChangesByRecord">#</a> **getEntityRecordChangesByRecord** | |||
There was a problem hiding this comment.
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.
| { name: 'site', kind: 'root', baseURL: '/wp/v2/settings' }, | ||
| { name: 'postType', kind: 'root', key: 'slug', baseURL: '/wp/v2/types' }, | ||
| { | ||
| label: __( 'Site' ), |
There was a problem hiding this comment.
I had to introduce this "label" property and also a dependency towards i18n package.
| transientEdits: { | ||
| blocks: true, | ||
| selectionStart: true, | ||
| selectionEnd: true, | ||
| }, | ||
| mergedEdits: { meta: true }, | ||
| getTitle( record ) { |
There was a problem hiding this comment.
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.
|
Size Change: -488 B (0%) Total Size: 865 kB
ℹ️ View Unchanged
|
packages/core-data/src/selectors.js
Outdated
| ).filter( ( pks ) => | ||
| hasEditsForEntityRecord( state, kind, name, pks ) |
There was a problem hiding this comment.
| ).filter( ( pks ) => | |
| hasEditsForEntityRecord( state, kind, name, pks ) | |
| ).filter( ( primaryKey ) => | |
| hasEditsForEntityRecord( state, kind, name, primaryKey ) |
packages/core-data/src/selectors.js
Outdated
| primaryKey | ||
| ); | ||
| dirtyRecords.push( { | ||
| // We avoid using primrayKey because it's transformed to a string |
There was a problem hiding this comment.
| // We avoid using primrayKey because it's transformed to a string | |
| // We avoid using primaryKey because it's transformed into a string |
packages/core-data/src/selectors.js
Outdated
| dirtyRecords.push( { | ||
| // We avoid using primrayKey because it's transformed to a string | ||
| // when it's used as an object key. | ||
| key: entityRecord[ entity.key || 'id' ], |
There was a problem hiding this comment.
Let's use the constant for the default entity key here.
| ); | ||
| } | ||
| ); | ||
|
|
||
| entitiesToSave.forEach( ( { key, kind, name } ) => { |
There was a problem hiding this comment.
| entitiesToSave.forEach( ( { key, kind, name } ) => { | |
| entitiesToSave.forEach( ( { kind, name, key } ) => { |
It'd be nicer to have all these in this order for consistency.
| { record.entity.label } | ||
| { !! record.title && ( |
There was a problem hiding this comment.
Why is the title a top-level item, but the label is merged into the entity? I think both should be top-level as the label is not part of the entity as provided by the API.
There was a problem hiding this comment.
Ideally, "entity" is removed entirely from this object (it duplicates kind, name) and we should use getEntity selector. Laziness made me add it to the experimental selector.
| import classnames from 'classnames'; | ||
| import memoize from 'memize'; | ||
| import EquivalentKeyMap from 'equivalent-key-map'; |
There was a problem hiding this comment.
Maybe we can remove this dep now?
|
Could you help me understand what the "Site" item is in this confirmation? |
|
@shaunandrews That's when a site setting changes (like Site title) |
@youknowriad could it be feasible to use the block name that originated the change perhaps (in this case it'd say "Site Title")? |
Two potential issues here: A property can be bound/edited to multiple blocks. We don't have the information anywhere saying "this block modified this property". I wonder if we can just use "Site Settings" as the entity label in this case. |
|
What about "Name of Block (Settings)"? I think a block changing a lot of these is going to be the exception rather than the rule, and the block name would be a lot more clear to the user. |
The main issue here is that this UI is built in a generic way at the moment. It shows any modified entity (including third-party ones).
An alternative here would be to rework that UI to be a specific UI. Say in the component that we only save site, templates and posts and have a specific UI for each entity. |
| @@ -22,5 +22,5 @@ | |||
| } | |||
|
|
|||
| .components-base-control + .components-base-control { | |||
| margin-bottom: $grid-unit-20; | |||
| margin-top: $grid-unit-20; | |||
There was a problem hiding this comment.
This caused the following regression:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 .
There was a problem hiding this comment.
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.
refs #20421
This PR addresses the first step of #20421 and tries to have more user-friendly labels. It is though very hard to achieve in a generic way and this PR had to also introduce some changes to existing APIs.