Global style revisions: show change summary on selected item #56577
Conversation
|
Size Change: -13 kB (-1%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
try/global-style-revisions-item-expand
branch
from
7025377
to
1cfebc7
Compare
|
Flaky tests detected in 25b8a15. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7162830527
|
There was a problem hiding this comment.
This feel pretty promising to me! It's quite nice even having a few words when clicking between each revision.
One thought I had was whether it'd be possible to drop style or styles or color and colors and group by type, if it helps shorten things a little? Not a worry if that would make things too complex, though.
But it's already feeling pretty good to me so far 🙂
packages/edit-site/src/components/global-styles/screen-revisions/get-revision-changes.js
Outdated
Show resolved
Hide resolved
| 'spacing.margin': __( 'margin styles' ), | ||
| 'spacing.padding': __( 'padding styles' ), | ||
| 'spacing.blockGap': __( 'block gap' ), | ||
| 'settings.layout': __( 'layout settings' ), |
There was a problem hiding this comment.
From a user perspective, the only things that will have changed here is content or wide size, I think? So settings might not feel very direct to the user. We could either split this out into content/wide size, or use layout styles for consistency?
Another idea, though, could be to remove the word style or styles from each of these strings, and use style or plural styles at the end of the text string?
There was a problem hiding this comment.
Oh yes, fair point. I like layout styles.
could be to remove the word style or styles from each of these strings, and use style or plural styles at the end of the text string?
I like this idea. I'll have a play around with it - I just need to make sure it plays well with translatable strings. If not, I think having "styles" after each higher-level style group is okay.
packages/edit-site/src/components/global-styles/screen-revisions/get-revision-changes.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/screen-revisions/get-revision-changes.js
Outdated
Show resolved
Hide resolved
ramonjd
force-pushed
the
try/global-style-revisions-item-expand
branch
from
cc2035c
to
7692a7e
Compare
ramonjd
changed the title
ramonjd
added
Global Styles
ramonjd
force-pushed
the
try/global-style-revisions-item-expand
branch
from
3478160
to
6b1790d
Compare
| if ( ! isObject( changedObject ) && ! isObject( originalObject ) ) { | ||
| // Only return a path if the value has changed. | ||
| // And then only the path name up to 2 levels deep. | ||
| return changedObject !== originalObject |
There was a problem hiding this comment.
So this is very specific to the UI's requirements.
We're just returning the path to a value that has changed between versions.
If we want to reuse this function later, we can return anything, e.g., an object:
{
path: 'styles.typography.fontWeight',
oldValue: 600,
newValue: 300,
}
| previousRevision, | ||
| blockNames | ||
| ) { | ||
| const cacheKey = JSON.stringify( { revision, previousRevision } ); |
There was a problem hiding this comment.
This might be an overkill, but it's the only way I think of to ensure that we can return the right cached results for a matching set of revision and previousRevision.
There was a problem hiding this comment.
This is looking good. I think it's a nice improvement and gives that extra bit of information.
I do wonder if we need to have some sort of context as to what the words mean. Do you think saying "Styles changed: xyz" would be better?
I noticed a couple of things. The text should line up and not be inset slightly. Also does the ellipsis mean data has been truncated?
|
I see it does mean it's truncated |
|
Thanks for testing!
Yeah, the idea is to indicate that there are more changes than the UI allows. I've set an arbitrary limit of 7, but that was only eye-balling it. The ellipsis is kind of clumsily thrown in there, and I'm not really sure how to represent the concept of "more changes" and also make it translator-friendly. Maybe there's an alternative way to present the summary, or we ditch the ellipsis altogether. I was just concerned that the summary might not match what the user expects, so if there's an ellipsis or "more" hint in there it'll communicate that there are more changes.
Worth trying out 👍🏻 Maybe playing around here might lead to something clearer in light of the truncation comment above. |
|
@apeatling What do you think about this? 
Forgetting everything else in that screenshot, maybe it'd be useful to somewhere show the truncated results and how many more there are ( |
|
Oh, I like that list idea, makes it easier to see changes at a glance compared to a paragraph / sentence based structure. |
|
|
||
| /** | ||
| * Get an array of translated summarized global styles changes. | ||
| * Results are cached using a WeakMap key of `{ revision, previousRevision }`. |
There was a problem hiding this comment.
Tiny nit: this mentions WeakMap but at the beginning of the file, it appears to be a Map.
There was a problem hiding this comment.
Good spotting. 👍🏻 I swapped from WeakMap to Map because of the change in key.
|
Just another idea, if we wanted to find a mid-point between a list and sentence structure (if a list winds up being too long), could we kind of group them by type, to remove repetition of the word 
|
I like this because it removes the need to append "block" to the block label, which might be dodgy for i18n. 🙇🏻 |
|
|
||
| /** | ||
| * Get an array of translated summarized global styles changes. | ||
| * Results are cached using a WeakMap key of `{ revision, previousRevision }`. |
There was a problem hiding this comment.
| * Results are cached using a WeakMap key of `{ revision, previousRevision }`. | |
| * Results are cached using a Map() key of `JSON.stringify( { revision, previousRevision } )`. |
|
I have a couple of commits lined up. 1 - 65833c9 (latest)Here's the one with the grouping, which required a bit of a code refactor, but simplifies the translations a lot. 2023-12-07.15.13.08.mp4And without a heading: 
It's all tweakable 😄 2 - ec31842And the other commit mentioned above. cc @jameskoster for 🎨 👀 |
rejigging getLabel function adding css var
… the editor state.
ramonjd
force-pushed
the
try/global-style-revisions-item-expand
branch
from
65833c9
to
ec31842
Compare
Thanks for testing! Yeah, I'm now in UI bike shedding territory 😄 |
|
Thanks for the ping, and nice work so far. My initial thought is that we probably don't need to be so granular for typography, styles, and colors. It might be enough to simply list each one accordingly which would reduce a lot of noise. In a similar vein I'd agree with removing the heading. Blocks can continue to be listed.
What do you think? |
|
Thanks for the effort here @ramonjd. I gave the current version a spin, and had a look at your efforts above. To be honest I think with the extra detail the scan-ability is lost, and adds extra noise like @jameskoster mentioned. I think what we had originally is actually better, and we can see if the context issue comes up as an real problem once it's out there. |
This helps a lot, thanks, folks. Easy to revert since it's what we started with, and I'd suspect it'd be a little more performant on the client side too with not complicated component rendering. Cheers! |
|
+1 on reducing noise for now and iterating as feedback comes in! |
Added e2e assertion
| @@ -215,14 +216,6 @@ function ScreenRevisions() { | |||
| </ConfirmDialog> | |||
| ) } | |||
| </> | |||
| ) : ( | |||
| <Spacer marginX={ 4 } data-testid="global-styles-no-revisions"> | |||
There was a problem hiding this comment.
This was dead code because we never show the panel where there are no revision.
|
I think I've addressed all the latest feedback, thanks for your input 🙇🏻 |
|
Also wondering, would it be useful to have a link to 
|
|
Looking good. A couple of suggestions:
Potentially worth exploring separately. It's a tricky one since it kicks you out of the site editor. |
So something like
👍🏻 I'll test it out here.
This was supposed to indicate that the styles match what's in the editor, and hence that's why they can't be applied. See comment: #55647 (comment) Maybe we can leave it it out and pursue this in a follow up. It's not directly related to this PR.
Ah, good call. I'll make a note of it. |
Moving everything into the button so it's clickable.
Done that and updated PR description 🙇🏻 |
No, not quite. I'm suggesting to align the listed changes with the sections titles in the Styles panel. So if there are any changes in the "Typography" panel then the revision simply lists "Typography" as a change. Same for Colors and Layout. For blocks, each edited block is listed. So if I changed the Link element, adjusted global block spacing, and added a border radius to the Image block the change list would be: "Typography, Layout, Image block".
I think that's a good idea, it needs a little design exploration. |
I get you, thanks for the clarification. I think this also should be doable 😄 |
There was a problem hiding this comment.
I think this is looking good for an initial effort and is a definite improvement, so let's get it in and we can iterate more from here. 👍
+1 this looks like a great place to merge for now and see how it feels for folks. Definitely feels nice to me in its current form, and the code looks good!
2023-12-13.09.08.09.mp4
✨
|
Thanks again for sticking with me on this one, folks 🙇🏻 |
What?
Maybe one day resolves #55647
Part of:
2023-12-11.15.04.39.mp4
Why?
To give the user a short overview of the global styles revision change set, which may not be evident from the canvas preview.
Also, highlighting revisions that match current editor styles by a left-hand-side dark, border.
TODO
How?
Block name > styleTesting Instructions
Make a bunch of changes to your global styles revisions, ensuring to get a decent mix of block, top-level, element styles and layout, typography and color settings.
If the theme, e.g., 2024, has style variations, activate and save a few revision using those.
Open up the styles revisions panel and check that the change summary makes sense.