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

Simplify the name input edit field (editor view) #12

Merged
merged 4 commits into from
Jan 18, 2023

Conversation

mike-day
Copy link
Contributor

This PR aims to cleanup the experience of using the name input field by removing the unneeded buttons and disabled state.

Now, the input looks and behaves like a normal input field, but with name collision warnings:

pattern-editor-name-collision


Note that empty titles are not allowed to be written to postMeta. If the name edit input is empty and the user clicks away from the field, the input value will automatically be set to the previous name.

Additionally, this change fixes a previously unknown bug! Before, pressing the escape key while the field was empty caused the input to disappear altogether. This behavior has been corrected.


How to test

  1. Checkout the branch
  2. Edit a pattern
  3. Open the Pattern Title sidebar panel if closed
  4. Start typing in the revamped input field

Comment on lines +95 to +98
handleChange( 'title', nameInput, {
name: convertToSlug( nameInput ),
previousName: previousPatternName.current,
} );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Firing handleChange inside of onBlur means that the new title will not be written to postMeta until the input element loses focus. This will happen when the user clicks outside of the input, hits the save button, etc.

Using this method allows basic validation before writing anything to postMeta, meaning we can keep from writing an empty field, or even expand validation to catch certain illegal characters, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea, as onChange() will happen before onBlur()

Comment on lines -79 to -80
<div className="patternmanager-pattern-post-name-input-outer">
<div onDoubleClick={ () => setNameInputDisabled( false ) }>
Copy link
Contributor Author

@mike-day mike-day Jan 17, 2023

Choose a reason for hiding this comment

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

Feels good to delete this markup and the unneeded buttons below!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes!!

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,
Nice work. It's easier to change the pattern title, and the validation still works, like before:

valid-another

@@ -0,0 +1,7 @@
/** Converts a string to a slug. */
export default function convertToSlug( toConvert = '' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice!

Could this also add the unit test for that function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh whoops, I missed that!

Copy link
Contributor

Choose a reason for hiding this comment

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

All good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you think about moving at least some common utils to a dir that sits on the wp-modules level? I am already reusing a few of these between modules — maybe we could prevent some code duplication that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That idea would be something for another PR, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, yes!

Sorry, don't worry about the unit test for this, I forgot that it's a copy-paste from wp-modules/app/

Copy link
Contributor

@kienstra kienstra Jan 17, 2023

Choose a reason for hiding this comment

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

The wp-modules/ all require a .php file in them to avoid an error.

So we could keep all of the common utils in wp-modules/app/js/src/utils/, and import them in wp-modules/pattern-post-type/ like how the Gutenberg packages/components/ directory imports from the packages/element/ directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that's an interesting idea! I am going to play around with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've got this working locally and it's a really nice touch! To keep the scope of this PR clear, I am going to merge the work done here and open a new PR for using utils from app via package.json.

Copy link
Contributor

Choose a reason for hiding this comment

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

...I am going to merge the work done here and open a new PR for using utils from app via package.json.

Oooh nice! Perfect!

@@ -12,7 +13,6 @@ export default function TitlePanel( {
handleChange,
}: BaseSidebarProps ) {
const [ nameInput, setNameInput ] = useState( '' );
const [ nameInputDisabled, setNameInputDisabled ] = useState( true );
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes!

* Set nameInput and inputDisabled state when the post is switched.
* Mostly intended to catch switching between patterns.
* Set nameInput and inputDisabled state when postMeta is loaded.
* Also intended to catch switching between patterns.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work keeping the docs up to date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I was hoping to get rid of the useEffect to which this doc block refers, but no such luck yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, haha!

Comment on lines -79 to -80
<div className="patternmanager-pattern-post-name-input-outer">
<div onDoubleClick={ () => setNameInputDisabled( false ) }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes!!

// Validate the nameInput to provide immediate feedback.
checkPatternTitle( value );
} }
onBlur={ () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to do this in onBlur(). No need to set the post meta on every change event.

Comment on lines +95 to +98
handleChange( 'title', nameInput, {
name: convertToSlug( nameInput ),
previousName: previousPatternName.current,
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea, as onChange() will happen before onBlur()

@mike-day mike-day merged commit b63950c into main Jan 18, 2023
@mike-day mike-day deleted the refactor/name-input-field branch January 18, 2023 02:39
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