-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FormTokenField: Hide suggestions list on blur event if input value is invalid #48785
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 |
Thanks for the ping. This makes sense to me in principle but I wasn't able to test in Storybook because I cannot set |
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 ),
+};
Oh, it seems the wrong rebase. Sorry 😞 |
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 <marco.ciampo@gmail.com>
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. |
Ah! Sorry about that. Looks like that connection was missed for you in our collection process. I'll update the list to reflect that. Thank you for your contribution! |
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
__experimentalValidateInput
property.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?
inputHasValidValue
is a bit misleading now because we have__experimentalValidateInput
. Basically, the first one usessaveTransform
under 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
__experimentalValidateInput
property. 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
Tab
button.Screencast
Before
before.webm
After
after.webm