Skip to content
This repository has been archived by the owner on Oct 16, 2024. It is now read-only.

Bug fix β€” faulty title collision caused by autosave #88

Merged
merged 8 commits into from
Mar 6, 2023

Conversation

mike-day
Copy link
Contributor

@mike-day mike-day commented Mar 3, 2023

Currently, there is a tricky little bug involving autosave that is annoying to recreate πŸ˜†

Lets say that you rename a pattern but do not click save, opting instead to change other meta values. But then you change your mind about renaming the pattern, deciding instead to stick with the title you already had.

Upon retyping the old pattern title, if autosave has saved your work, the initial title will improperly error as a name collision.

This PR corrects that by getting the currentName (equivalent to the slug) from getCurrentPostAttribute for checking against the title. Using this method avoids needing to use a ref, but works in a similar way.


How to test

  1. Checkout the branch
  2. Open a pattern
  3. Rename the pattern, but do not click Update Pattern
  4. Wait for an autosave
  5. Type the old pattern name β€” there should be no collision


export default function PatternManagerMetaControls() {
const { postMeta, title } = useEditedPostData();
const { currentName } = useSavedPostData();
Copy link
Contributor Author

@mike-day mike-day Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new hook that I think could make sense to keep. The saved data is inherently different from the edited data since we are editing data directly (via editPost for example) as values change.

The saved and edited data won't necessarily match until the post is actually saved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far. It keeps the same level of abstraction as useEditedPostData()

@@ -56,6 +59,7 @@ export default function PatternManagerMetaControls() {
postMeta={ postMeta }
handleChange={ updatePostMeta }
errorMessage={ errorMessage }
currentName={ currentName }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing in currentName here behaves somewhat like a ref for our purposes.

Comment on lines -19 to -21
const savedPatternName = useSelect( ( select: SelectQuery ) => {
return select( 'core/editor' ).getCurrentPostAttribute( 'meta' )?.name;
}, [] );
Copy link
Contributor Author

@mike-day mike-day Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer needed β€” the new hook uses this call to get currentName.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!!

@mike-day
Copy link
Contributor Author

mike-day commented Mar 3, 2023

postMeta is still passed to TitlePanel to satisfy typing, but it is no longer used in the component.

I think we should consider rethinking the passing of postMeta to every component, instead getting the values we need from useEditedPostData and passing them directly as props.

For example, instead of this:

<KeywordsPanel
    postMeta={ postMeta }
    handleChange={ updatePostMeta }
/>
<DescriptionPanel
    postMeta={ postMeta }
    handleChange={ updatePostMeta }
/>

...we could do something like this:

<KeywordsPanel
    keywords={ metaKeywords }
    handleChange={ updatePostMeta }
/>
<DescriptionPanel
    description={ metaDescription }
    handleChange={ updatePostMeta }
/>

Or we could also use context instead of passing these props around.

@kienstra
Copy link
Contributor

kienstra commented Mar 3, 2023

Interesting! I'll look at this later today if that's alright.

@mike-day
Copy link
Contributor Author

mike-day commented Mar 3, 2023

Interesting! I'll look at this later today if that's alright.

Of course! No rush on this one β€” I only noticed from running my demo a bunch of times.

Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mike-day,
Really nice fix. With your 'How to test' steps, there's no title collision message anymore:
Screenshot 2023-03-03 at 4 53 10 PM

@kienstra
Copy link
Contributor

kienstra commented Mar 3, 2023

I think we should consider rethinking the passing of postMeta to every component, instead getting the values we need from useEditedPostData and passing them directly as props.

Good idea. The components should only accept the meta value they need, not all of postMeta.

I'd say it's fine to keep passing them as props, instead of putting them in global context.

@mike-day
Copy link
Contributor Author

mike-day commented Mar 5, 2023

@kienstra please check me on this, but I also noticed that we don't seem to use title from postMeta anymore β€” we are getting the actual post title, so that meta property is not needed.

I went ahead and removed that property as well with 0d46e71.

@mike-day
Copy link
Contributor Author

mike-day commented Mar 5, 2023

I think we should consider rethinking the passing of postMeta to every component, instead getting the values we need from useEditedPostData and passing them directly as props.

Good idea. The components should only accept the meta value they need, not all of postMeta.

I'd say it's fine to keep passing them as props, instead putting them in global context.

Cool, I think it will be a bit cleaner that way! I am going to complete that work in a new PR after merging this one.

@kienstra
Copy link
Contributor

kienstra commented Mar 5, 2023

@kienstra please check me on this, but I also noticed that we don't seem to use title from postMeta anymore β€” we are getting the actual post title, so that meta property is not needed.

That's right, good catch.

@@ -46,7 +50,7 @@ export default function TitlePanel( {
'pattern-manager'
) }
value={ title }
onChange={ ( newTitle: PostMeta[ 'title' ] ) => {
onChange={ ( newTitle: typeof title ) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one, I wouldn't have thought of that

@mike-day mike-day merged commit 0023596 into main Mar 6, 2023
@mike-day mike-day deleted the fix/autosave-title-faulty-collision branch March 6, 2023 15:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants