GlobalStyles: Save and Delete JS changes #47076
Conversation
ribaricplusplus
requested review from
ntwb,
nerrad and
ajitbohra
as code owners
ribaricplusplus
added
[Type] Enhancement
ribaricplusplus
force-pushed
the
rpp-js-changes
branch
from
5db4c0e
to
19a9cf6
Compare
|
Size Change: +1.59 kB (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Thank you for working on this! Took it for a spin. In this GIF, navigating to Global Styles > Browse styles, then using the new button to save a custom style:
As far as superficial quick testing, this feels like it's working fairly well, and validates for me a lot of the ideas that we originally proposed in only a static mockup. I'd love additional input by @WordPress/gutenberg-design, but so far this feels like a good place to start. Depending on that feedback, and perhaps not something that has to be fixed in this partcular PR, there are a number of smaller visual issues that could be refined. This button's footprint is too big. It should be 24x24 just like this one:
(Though I still think it's correct to use the horizontal ellipsis here, to visually reduce the footprint and align with the thumbnail above) Same with this button, it should also be 24x24:
Conceptually that plus comes from this area of Global Styles, and should have similar metrics with padding and placement: The buttons on the right should also align vertically: Note that the X for the inspector not vertically aligning, this is a separate and recent bug, they align in the recent plugin release: If possible, it'd be good to remove this separating line:
The padding left and right of the styles thumbnails seems bigger than it should be:
It should be similar to the top level: ☝️ Again, some of the above may be existing issues, in which case they shouldn't block these efforts. Thanks again for the work! |
|
@jasmussen Thanks for taking a look! I'm going to address the styling issues you mentioned. |
|
Flaky tests detected in 4a6b7d7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3975820680
|
|
Rapidly coming along, nice. Took it for a quick spin again:
Looks close. There's still some padding stuff that's off. Trunk you see the thumbnails align with the text:
In this one they are slightly indented:
Also I noticed that when you click the plus to add a new style, I can type in the text but not press "Enter", I have to set focus on the Create button. The horizontal ellipsis button could also still be the 24x24px small version. These last details could maybe use a little component input from @ciampo or @mirka if they have time. |
|
@jasmussen I have to point out that my priority is currently on getting #47075 merged, since this PR cannot work without that one getting merged first. As soon as that is done I'm going to refocus on the styling / a11y changes that are needed here. |
|
Oh absolutely! Didn't mean to stress this point, and thank you for your contributions. |
|
Thank you for the ping, @jasmussen !
Probably the best way to achieve the desired behaviour here would be use a native While taking a look, I also noticed that:
I put together these code changes on my machine, what do folks think?diff --git a/packages/edit-site/src/components/global-styles/screen-style-variations.js b/packages/edit-site/src/components/global-styles/screen-style-variations.js
index 2fcdca5e79..f34cc84c61 100644
--- a/packages/edit-site/src/components/global-styles/screen-style-variations.js
+++ b/packages/edit-site/src/components/global-styles/screen-style-variations.js
@@ -354,6 +354,8 @@ function ScreenStyleVariations() {
const createNewStyleRecord = useCreateNewStyleRecord( newStyleName );
+ const isNewStyleNameEmpty = ( newStyleName ?? '' ).trim().length === 0;
+
return (
<>
<ScreenHeader
@@ -422,37 +424,51 @@ function ScreenStyleVariations() {
setCreateNewVariationModalOpen( false )
}
>
- <div className="edit-site-global-styles__cs-content">
+ <form
+ className="edit-site-global-styles__cs-content"
+ onSubmit={ () => {
+ // Avoid creating the new style:
+ // - in case the form gets submitted repeatedly
+ // - in case of an empty string for the name
+ if ( isStyleRecordSaving || isNewStyleNameEmpty ) {
+ return;
+ }
+
+ createNewStyleRecord().then( ( variation ) => {
+ // Set new variation as the associated one.
+ if ( variation?.id ) {
+ setUserConfig( ( currentConfig ) => ( {
+ ...currentConfig,
+ associated_style_id: variation.id,
+ } ) );
+ }
+
+ setIsStyleRecordSaving( false );
+ setCreateNewVariationModalOpen( false );
+ setNewStyleName( '' );
+ } );
+ setIsStyleRecordSaving( true );
+ } }
+ >
<InputControl
label={ __( 'Style name' ) }
value={ newStyleName }
onChange={ ( nextValue ) =>
setNewStyleName( nextValue ?? '' )
}
+ required
/>
<Button
- onClick={ () => {
- createNewStyleRecord().then( ( variation ) => {
- // Set new variation as the associated one.
- if ( variation?.id ) {
- setUserConfig( ( currentConfig ) => ( {
- ...currentConfig,
- associated_style_id: variation.id,
- } ) );
- }
-
- setIsStyleRecordSaving( false );
- setCreateNewVariationModalOpen( false );
- setNewStyleName( '' );
- } );
- setIsStyleRecordSaving( true );
- } }
variant="primary"
+ disabled={
+ isStyleRecordSaving || isNewStyleNameEmpty
+ }
isBusy={ isStyleRecordSaving }
+ type="submit"
>
{ __( 'Create' ) }
</Button>
- </div>
+ </form>
</Modal>
) }
</>I would also consider moving the
By looking at the code, it looks like:
This should do it (`24` can be changed to whatever icon size is needed):diff --git a/packages/edit-site/src/components/global-styles/screen-style-variations.js b/packages/edit-site/src/components/global-styles/screen-style-variations.js
index f34cc84c61..3acd931cf5 100644
--- a/packages/edit-site/src/components/global-styles/screen-style-variations.js
+++ b/packages/edit-site/src/components/global-styles/screen-style-variations.js
@@ -265,7 +265,7 @@ function UserVariation( { variation, userChangesMatchAnyVariation } ) {
/>
</GlobalStylesContext.Provider>
</div>
- <MoreMenuDropdown>
+ <MoreMenuDropdown toggleProps={ { iconSize: 24 } }>
{ () => (
<MenuGroup>
<MenuItem onClick={ deleteStyleHandler }> |
| // Delete all user global styles | ||
| await requestUtils.rest( { | ||
| method: 'DELETE', | ||
| path: '/wp/v2/delete-all-global-styles', | ||
| } ); |
There was a problem hiding this comment.
Do you think it's worth adding this as a util in request utils directly? Like the deleteAllTemplates util above.
| '.components-card__body > .components-flex > .components-button' | ||
| ) | ||
| .click(); | ||
| await page.getByLabel( 'Style name' ).click(); |
There was a problem hiding this comment.
Nit: getByRole is usually preferred over other getBy* queries.
| await page.getByRole( 'button', { name: 'Browse styles' } ).click(); | ||
| await page | ||
| .locator( | ||
| '.components-card__body > .components-flex > .components-button' |
There was a problem hiding this comment.
Is there a more accessible selector here we can use? Preferably getByRole? If not, then can we fix it in the source code to make it more accessible (if it's in the scope of this PR)?
There was a problem hiding this comment.
Will work on this
There was a problem hiding this comment.
the Button component should normally have the button or link role, depending on whether an href prop is passed.
|
|
||
| export function compareVariations( a, b ) { | ||
| if ( ! a.styles ) { | ||
| a.styles = {}; |
There was a problem hiding this comment.
This alters the original object a. It's generally a good idea to keep most utils pure. Do you think the same rule applies here?
There was a problem hiding this comment.
Excellent point, thank you, I will change this
|
@jasmussen I'm down to implement requested changes here and resolve conflicts that by now exist with trunk. However, resolving conflicts again and again is not something I can do, so I would like it if we could have quicker iteration and get this merged. The current bottleneck is getting people to review my code. |
|
Yep, totally understand, that's why I'd love to bring some more attention to this first, ideally with some specific action items to get this landed. Thank you for your contributions 🙏 |
| * @param {*} recordId Record ID. | ||
| * | ||
| */ | ||
| export const __experimentalDiscardRecordChanges = |
There was a problem hiding this comment.
What is this, a sort of undo? Should we just refetch the original instead?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -19,6 +19,8 @@ import { unlock } from '../../experiments'; | |||
|
|
|||
| const { GlobalStylesContext } = unlock( blockEditorExperiments ); | |||
|
|
|||
| /* eslint-disable dot-notation, camelcase */ | |||
There was a problem hiding this comment.
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.
| @@ -62,40 +69,74 @@ function useGlobalStylesUserConfig() { | |||
| _globalStylesId | |||
| ) | |||
| : undefined; | |||
| const _associatedStyleId = record | |||
There was a problem hiding this comment.
As I commented in the PHP PR as well, we need to explain in the comments what associated styles are.
| hasFinishedResolution( | ||
| '__experimentalGetCurrentGlobalStylesId' | ||
| ) | ||
| ) { | ||
| hasResolved = _globalStylesId | ||
| ? hasFinishedResolution( 'getEditedEntityRecord', [ | ||
| hasResolved = ( () => { |
There was a problem hiding this comment.
I don't understand what isReady does that hasResolved doesn't? Is it a sort of Promise.all()?
| ) { | ||
| let numTries = 0; | ||
| const MAX_NUM_TRIES = 30; | ||
| const intervalId = setInterval( () => { |
There was a problem hiding this comment.
Why do we need to retry 30 times?
Part of #46952. JavaScript changes required to make the main PR work. The base for this PR is the PR that introduces the required PHP changes: #47075. It should not be merged into trunk before #47075.