⚡ NEW: WordPress/gutenberg/pull/ - Full Archive
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Conversation
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @shvlv! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
|
Thank you for working on this, @shvlv ! I appreciate the detailed PR description and the addition of unit tests! Before continuing in the review process, I'd love to hear opinions from design folks too, @jasmussen @jameskoster @SaxonF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 This change makes sense to me, I agree that it's a nice UI improvement. The changes look good and test well for me - thanks for adding tests at the same time.
I tested in Storybook by locally adding a quick story to trial the prop:
diff --git a/packages/components/src/form-token-field/stories/index.tsx b/packages/components/src/form-token-field/stories/index.tsx
index d4af720913..51997ba17b 100644
--- a/packages/components/src/form-token-field/stories/index.tsx
+++ b/packages/components/src/form-token-field/stories/index.tsx
@@ -123,3 +123,11 @@ WithCustomRenderItem.args = {
<div>{ `${ item } — a nice place to visit` }</div>
),
};
+
+export const WithValidatedInput: ComponentStory< typeof FormTokenField > =
+ DefaultTemplate.bind( {} );
+WithValidatedInput.args = {
+ ...Default.args,
+ __experimentalValidateInput: ( input: string ) =>
+ continents.includes( input ),
+};
I'm not sure if we'd actually want to add a story like that for an experimental prop though...
I'll defer to @mirka or @ciampo on the actual approval, they have better eyes for our usual testing patterns than I do.
|
Thank you @jameskoster and @chad1008 ! I'll go ahead and take a deeper look at the code changes. @shvlv , in the meantime could you add a "before" and "after" screen recording of the component, so that more folks are going to be able to understand the result of the proposed changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work here, @shvlv !
Could you add an entry to the package's CHANGELOG ?
@chad1008 's suggestion makes sense to me. We could add a new storybook example that showcases how to use the `__experimentalValidateInput` props
diff --git a/packages/components/src/form-token-field/stories/index.tsx b/packages/components/src/form-token-field/stories/index.tsx
index d4af720913..0657e49917 100644
--- a/packages/components/src/form-token-field/stories/index.tsx
+++ b/packages/components/src/form-token-field/stories/index.tsx
@@ -123,3 +123,17 @@ WithCustomRenderItem.args = {
<div>{ `${ item } — a nice place to visit` }</div>
),
};
+
+/**
+ * Only values for which the `__experimentalValidateInput` function returns
+ * `true` will be tokenized. (This is still an experimental feature and is
+ * subject to change.)
+ */
+export const WithCustomInputValidation: ComponentStory<
+ typeof FormTokenField
+> = DefaultTemplate.bind( {} );
+WithCustomInputValidation.args = {
+ ...Default.args,
+ __experimentalValidateInput: ( input: string ) =>
+ continents.includes( input ),
+};
c12465f to
9c61286
Compare
|
Hi, @ciampo I've applied your suggestions and updated the PR description with before/after small screencasts. Let me know if something else is needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Before merging, would you mind also applying my previous suggestion about adding an additional Storybook example?
Thank you 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit confusing to me because visual code representation doesn't contain the passed callback but it looks like the Storybook problem 🤷♂️
That's correct — it is a known Storybook bug (I couldn't find the exact issue but I've read about it before).
Co-authored-by: Marco Ciampini <[email protected]>
|
Thank you again for your contribution, @shvlv 🙏 |
|
Congratulations on your first merged pull request, @shvlv! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
|
@shvlv If you have a moment, could you connect your GitHub account to your wordpress.org one? This change is slated to be included in the upcoming WordPress 6.3 release on August 8th, and it would be great if we could give you attribution by mentioning you on the credits screen. You can find more details on how contributions are tracked and how connecting GitHub accounts helps in this post on the Make WordPress Core blog. |


What?
Currently, if FormTokenField loses focus the suggestions list is still rendered unless the input value is empty. The PR forces input clearing and suggestion list closing on blur event even if the input is not empty but invalid based on the
__experimentalValidateInputproperty.Why?
In my opinion, the change allows for improving UX because it is not clear why the need suggestions list is still opened after blurring (i.e. the user finished interaction) if there is no valid information.
How?
inputHasValidValueis a bit misleading now because we have__experimentalValidateInput. Basically, the first one usessaveTransformunder the hood which is useful during suggestion searching and matching, and saving, the second one validates the token immediately before persisting.IMO
inputHasValidValue() && __experimentalValidateInput( incompleteTokenValue )is the right condition to detect if the input is "valid" (makes sense) after blurring.Testing Instructions
Changes could be reproduced if the FormTokenField component uses
__experimentalValidateInputproperty. In this case, if the input value doesn't pass the validation suggestion list gets hidden after the field loses focus.Testing Instructions for Keyboard
The change works if you switch to the next active element with the
Tabbutton.Screencast
Before
before.webm
After
after.webm