Global styles revisions: load unsaved revision item into the revisions preview #55880
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramonjd
added
[Type] Bug
|
Size Change: +103 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
andrewserong
reviewed
Nov 8, 2023
There was a problem hiding this comment.
This is testing nicely for me! Just left a few comments, mostly around readability of the useEffect and whether we can please the exhaustive deps linter. In practice it looks like the useEffect is firing when expected for me in manual testing 👍
Please let know if I'm overthinking any of my feedback here — I haven't looked at any code in the past couple of weeks so am a bit rusty in thinking through useEffects 🙂
Comment on lines
73
to
79
| /* | ||
| Ensure that the first item is selected where no revision is selected | ||
| and the selected styles don't match the current editor styles. | ||
| This is required in case editor styles are changed outside the revisions panel, | ||
| e.g., via the reset styles function of useGlobalStylesReset(). | ||
| See: https://github.com/WordPress/gutenberg/issues/55866 | ||
| */ |
There was a problem hiding this comment.
Comment style nit: should this comment have an asterisk at the beginning of each line for consistency with the JS multi-line comments guidelines?
Comment on lines
83
to
88
| }, [ | ||
| editorCanvasContainerView, | ||
| shouldSelectFirstItem, | ||
| revisions[ 0 ], | ||
| setGlobalStylesRevision, | ||
| ] ); |
There was a problem hiding this comment.
This dependency array is complaining about some missing dependencies:
To neaten things up, would it work to split out this new behaviour to a separate useEffect, that way the dependencies related to the editorCanvasConatinerView can be treated separately?
There was a problem hiding this comment.
Totally. I'll split them up to be neater.
| See: https://github.com/WordPress/gutenberg/issues/55866 | ||
| */ | ||
| if ( shouldSelectFirstItem ) { | ||
| setGlobalStylesRevision( revisions[ 0 ] ); |
There was a problem hiding this comment.
Does this also need to call setSelectedRevisionId as in selectRevision or is it enough to just update the revision?
There was a problem hiding this comment.
It's enough to update the revision as all we want to do is load the preview in the frame.
Having said that, I don't think it'd hurt to just call selectRevision() for consistency's sake.
👍🏻
| }, [ | ||
| editorCanvasContainerView, | ||
| shouldSelectFirstItem, | ||
| revisions[ 0 ], |
There was a problem hiding this comment.
I think using revisions seems to cause the exhaustive-deps rule to expect revisions in the dependency array somewhere. If we only need the first element, I wonder if we can grab the first element outside of the useEffect via const firstRevision = revisions[ 0 ] and use that? Just a thought!
There was a problem hiding this comment.
This could work, thanks!
I'll try it.
Comment on lines
62
to
64
| const shouldSelectFirstItem = | ||
| ! selectedRevisionMatchesEditorStyles && | ||
| ( ! selectedRevisionId || 'unsaved' === selectedRevisionId ); |
There was a problem hiding this comment.
areGlobalStyleConfigsEqual checks that globalStylesRevision matches userConfig, however the useEffect updates only the globalStylesRevision. Could there ever be a case when the useEffect goes into an infinite loop if the updated globalStylesRevision doesn't match the userConfig?
Or to put it differently, in the useEffect do we need to also check that the current globalStylesRevision is not equal to the first item in the list of revisions, to avoid a potential infinite loop? Apologies if I'm overthinking this!
There was a problem hiding this comment.
Or to put it differently, in the useEffect do we need to also check that the current globalStylesRevision is not equal to the first item in the list of revisions, to avoid a potential infinite loop?
Not overthinking at all. ! selectedRevisionId || 'unsaved' === selectedRevisionId implies that the first item is selected by default, but it wouldn't hurt to elaborate this defence. I'll have a play.
Thanks again for thinking about this one!
There was a problem hiding this comment.
areGlobalStyleConfigsEqual checks that globalStylesRevision matches userConfig, however the useEffect updates only the globalStylesRevision. Could there ever be a case when the useEffect goes into an infinite loop if the updated globalStylesRevision doesn't match the userConfig?
Thinking about that made me realize that the selectedRevisionId is 100% redundant since it's stored in (and saved and the same time as) globalStylesRevision.
So, here's what I have:
const shouldSelectFirstItem =
// If there is a revision to load in the first place,
!! firstRevision?.id &&
// AND if a revision is loaded in the preview pane that is
// different from the current editor state (userConfig),
! selectedRevisionMatchesEditorStyles &&
// AND if there's no currently-selected revision (a user hasn't clicked on anything)
// we can go ahead and select the first revision.
! currentlySelectedRevision?.id;What I'm trying to express here is that, if nothing has been selected, but for some reason the current state of the revisions pane doesn't reflect what's in the editor, let's fix that by selecting the first item in the list.
So, if globalStylesRevision doesn't match the userConfig and there's no revision selected in the list the useEffect condition will pass and then select the first revision using the state setter setCurrentlySelectedRevision.
The next time it spins around it will fail because it's selected a revision and we've double checked that an id exists for firstRevision?.id.
The only thing I can think of is if firstRevision?.id is falsey. But if that happens we've got bigger problems 😄
There was a problem hiding this comment.
Sounds good to me, thanks for laying that out!
- the user hasn't selected any revision in the list, or - the selected item is an unsaved revision, which the first one anyway, and - the selected styles don't match the current editor styles - added extra doc comments for context
formatting comments
ramonjd
force-pushed
the
try/global-styles-revisions-load-reset-styles
branch
from
3330528
to
8eb30b8
Compare
…on in globalStylesRevision
`userConfig` to `currentEditorGlobalStyles` `globalStylesRevision` to `currentlySelectedRevision` Use `setCurrentlySelectedRevision` state setter in the useEffect callback as it's a stable ref
andrewserong
approved these changes
Nov 9, 2023
There was a problem hiding this comment.
Thanks for the thorough follow-ups @ramonjd and for thinking through all the changes. This is looking really good to me now, and I like how it's both simplified the state a bit, while including easier to read names that more closely match the behaviour we're examining 👍
Still testing nicely for me:
2023-11-09.14.47.46.mp4
✅ Existing useEffect is still firing when expected to close out the panel and redirect to the root of global styles
✅ New useEffect fires only when expected and results in the unsaved revision item being updated as expected when resetting back to defaults
LGTM! ✨
packages/edit-site/src/components/global-styles/screen-revisions/index.js
Outdated
Show resolved
Hide resolved
…ns/index.js Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
cbravobernal
pushed a commit
that referenced
this pull request
Nov 14, 2023
…s preview (#55880) * Always load the first style object into the revisions preview: - the user hasn't selected any revision in the list, or - the selected item is an unsaved revision, which the first one anyway, and - the selected styles don't match the current editor styles - added extra doc comments for context * Splitting useEffect formatting comments * selectedRevisionId is redundant since we're saving the same information in globalStylesRevision * For clarity, rename: `userConfig` to `currentEditorGlobalStyles` `globalStylesRevision` to `currentlySelectedRevision` Use `setCurrentlySelectedRevision` state setter in the useEffect callback as it's a stable ref * Update packages/edit-site/src/components/global-styles/screen-revisions/index.js Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> --------- Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Resolves: #55866
Before
2023-11-05.18.13.00.mp4
After
2023-11-06.12.30.14.mp4
Why?
External changes to global styles in the editor, e.g., via the reset styles function of
useGlobalStylesReset(), will create a new "unsaved" entry in the revisions list and replace the first item in the list.However the styles aren't loaded in the preview.
How?
Always load the first style object into the revisions preview:
Testing Instructions
Testing Instructions for Keyboard