Global style revisions: redesign style revision items #55913
Conversation
ramonjd
added
[Type] Enhancement
|
Size Change: +657 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 387690e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6793350790
|
ramonjd
force-pushed
the
update/global-styles-revisions-buttons-details
branch
from
387690e to
e759258
Compare
- add basic function to return translated labels of top-level changes - showing humantime diff for changes < 1 day - rearranging elements
- minor optimization refactoring
ramonjd
force-pushed
the
update/global-styles-revisions-buttons-details
branch
from
e759258 to
064c6dc
Compare
| const displayDate = | ||
| modified && | ||
| dateNowInMs - modifiedDate.getTime() > DAY_IN_MILLISECONDS | ||
| ? dateI18n( dateFormat, modifiedDate ) |
There was a problem hiding this comment.
The date is formatted as OCTOBER 20 2023
This doesn't match the designs, which specify an abbreviated month — OCT 20 2023 — however I think it's important to introduce new formats using the date library and into Core so that themes can override them.
So TL;DR I can open a new PR and corresponding core patch to introduce the abbreviated month format if folks think it's worth it.
There was a problem hiding this comment.
I've noticed a bug where the previous revisions changes are added to the next revision. It's like the list isn't getting cleared. Try changing a couple of distinct things (other than typography), then save. Then change a global typography setting and save. You'll notice that the most recent revision lists not just typography changes.
In this example I only changed typography in the latest revision:
|
I also think we need to adjust the line height of the changes in the list to match everything else. You can see the difference between the line height in the description paragraph at the top, and the items in the list: 
|
| import { __ } from '@wordpress/i18n'; | ||
|
|
||
| const translationMap = { | ||
| blocks: __( 'Block styles' ), |
There was a problem hiding this comment.
Can we break out block names in a follow up? For example show "Paragraph block typography" etc instead of a generic "Block styles". I think block styles are going to change a bunch and a lot of "Block styles" being listed is not super useful.
There was a problem hiding this comment.
For blocks it makes sense. I think we have to put a limit on it at some point as it needs to be calculated for every revision item on render.
I'll see how memoizing/caching each one might work too.
@c4rl0sbr4v0 explored comparing the style/settings tree deeply: #52829 (comment)
Thanks for testing. It's a naive check in that it only describes an individual revision's changes. It doesn't do a comparison with the previous revision, which it probably should. I shied away from that approach for performance reasons. If we keep the comparison to the very top level it should be okay. I'll have a look. |
Actually, now that I reread that, we'd probably need to do a deep comparison to know what's changed, even at the top level. So for example if a revision makes changes to 🤔 |
|
@apeatling I've been testing various methods to get some kind of overview of changes. I think it's best for a follow up PR because it would contain a few changes and a lot of logic that would be good to have reviewed separately. For now I've removed the relevant code. Here are some initial thoughts for the follow up, assuming the goal of rendering a meaningful description of changes. Something like
I've taken @c4rl0sbr4v0's function from https://github.com/WordPress/gutenberg/pull/52829/files#diff-6baeb967c4bc1836222ed3912fb25cfdba460429895110e58ee23413e35498c6R97 and changed it a little:
It works okay, but needs a lot of refinement particularly surrounding which changes to show. Switching between style variation for example creates 100 changes, many of them settings changes. This makes for a lot of repetitiveness. So I believe we need to think and experiment a bit in order to balance all the concerns. |
|
Just catching up on the discussion — good idea skipping the complexity of comparing the changes of each revision for now, it does sound like a good thing to address separately and explore our options. One idea that came to me from reading the discussion: this might be ridiculous, but would an option be to potentially store a message in the |
There was a problem hiding this comment.
This is looking great to me in testing @ramonjd! Love the level of polish for the beginning and ends of the list so that the line never extends beyond the dot:
Beginning
End
Just left a few comments, mostly just tiny nits. The main thing I was wondering about was making sure we call getSettings() at render time so that if the date settings have been updated anywhere else, they're reflected in time.
Not for this PR, but looking at the repeated dates when many changes occurred on the one day makes we wonder if we should expose the time somewhere, too (or explore group-by-date collapsing), otherwise it can look a bit repetitive:
Not a blocker for now, though 🙂
packages/edit-site/src/components/global-styles/screen-revisions/revisions-buttons.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/screen-revisions/revisions-buttons.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/screen-revisions/style.scss
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/screen-revisions/style.scss
Show resolved
Hide resolved
|
Thanks for looking at this!
It's an interesting idea!
It did occur to me to move the work to the backend, though many of the questions in the above comment would still need to be answered. That is, I see the main challenge to be how and when to compile the data, and what that data would look like. If we saved an excerpt, we could avoid expensive object comparison by hooking into the global styles provider in the site editor and keep an audit of currently unsaved settings/style changes. Using that we could send a version of that data to the database when updating the global styles post. The catch is that any updates done over the API elsewhere might not compile such a change set, so it probably has to be done on the backend, in which case we push the performance burden to the server. Perhaps a dedicated endpoint to grab a change set might be more appropriate. To get back to the feature at a higher level, I had this question in my mind: What do we need to communicate in these revisions change summaries? In my view, if we're just dealing with copy, they should:
It's tricky! But if they're all reasonable assumptions, and we don't yet have a use case for a full change set (like in this design proposal), then I still think we could trial it clientside with the appropriate caching and failsafes to avoid going too deep into the tree. I'd be open to think about different UXs here too. E.g., clicking on the item to grab a changeset from the server and rendering the result. 🤷🏻 |
Agreed. Thanks for thinking this through in detail! I think a minimal clientside approach would be great to test out the ideas before baking anything in server-side 👍 |
rejigging getLabel function adding css var
Great idea!! I'll try it out. Agree that it looks a bit samey 😄 |
ramonjd
changed the title
packages/edit-site/src/components/global-styles/screen-revisions/style.scss
Show resolved
Hide resolved
What?
Resolves #55647
This PR:
2023-11-17.12.20.33.mp4
Testing Instructions
Now open the styles revisions panel.
Check that the revision items work and the active hover states are correct.