Components: FormTokenField - add prop to allow saving of tokens onBlur #53976
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
glendaviesnz
added
[Type] Enhancement
9 tasks
glendaviesnz
commented
Aug 28, 2023
| @@ -406,15 +410,15 @@ export function FormTokenField( props: FormTokenFieldProps ) { | |||
| } | |||
| } | |||
|
|
|||
| function addCurrentToken() { | |||
| function addCurrentToken( isOnBlur = false ) { | |||
There was a problem hiding this comment.
the isOnBlur param is needed as the setIsActive( false ); in the onBlur function is not registered until the next render so isActive is still true when addNewToken is called. There may be a better way of achieving this but couldn't think of one.
There was a problem hiding this comment.
We could use useEffect and react to isActive becoming false, but the way this component is written is quite entangled, and therefore we'd still face issues.
I wonder if we could use the hasFocus() function and the __experimentalAddOnBlur prop in the addNewToken function instead of passing a isOnBlur argument around?
There was a problem hiding this comment.
Refactored this here to get rid of isOnBlur as suggested and it seems to work, but will do some more testing tomorrow to make sure I didn't miss any edge cases.
|
Size Change: +25 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in f321e53. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6079330574
|
ciampo
reviewed
Sep 1, 2023
ciampo
reviewed
Sep 1, 2023
If you are happy with the general approach with the latest changes I will go ahead and add some additional unit tests tomorrow @ciampo |
|
Yup, the overall approach looks good to me! |
|
Unit tests have been added. |
aaronrobertshaw
approved these changes
Sep 5, 2023
There was a problem hiding this comment.
Thanks for this one @glendaviesnz, I often trip over the incomplete values not being tokenized, especially when testing pattern categories and taxonomies lately.
✅ Storybook examples function as intended
✅ The token field for pattern categories works as advertised
✅ Unit tests pass
I've left some minor tweaks and typo nits via inline comments. Also, the PR description is out of date after the renaming of the new prop, we should bring that up-to-date as well.
With the above tweaks made, I think this will be ready to land. Nice work!
Here's a diff with suggested tweaks and an extra typo fix that I couldn't add to this review in case it helps.
diff --git a/packages/components/src/form-token-field/README.md b/packages/components/src/form-token-field/README.md
index b442e42c31..9e31dcafeb 100644
--- a/packages/components/src/form-token-field/README.md
+++ b/packages/components/src/form-token-field/README.md
@@ -62,7 +62,7 @@ The `value` property is handled in a manner similar to controlled form component
- `__experimentalValidateInput` - If passed, all introduced values will be validated before being added as tokens.
- `__experimentalAutoSelectFirstMatch` - If true, the select the first matching suggestion when the user presses the Enter key (or space when tokenizeOnSpace is true).
- `__nextHasNoMarginBottom` - Start opting into the new margin-free styles that will become the default in a future version, currently scheduled to be WordPress 6.5. (The prop can be safely removed once this happens.)
-- `tokenizeOnBlur` - If true adds any incompleteTokenValue as a new token when field loses focus.
+- `tokenizeOnBlur` - If true, adds any incompleteTokenValue as a new token when field loses focus.
## Usage
diff --git a/packages/components/src/form-token-field/test/index.tsx b/packages/components/src/form-token-field/test/index.tsx
index 81e61ea186..c6a8397cb2 100644
--- a/packages/components/src/form-token-field/test/index.tsx
+++ b/packages/components/src/form-token-field/test/index.tsx
@@ -9,7 +9,6 @@ import {
within,
getDefaultNormalizer,
waitFor,
- act,
} from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import type { ComponentProps } from 'react';
@@ -219,9 +218,8 @@ describe( 'FormTokenField', () => {
// Add 'grapefruit' token by typing it and check blur of field does not tokenize it.
await user.type( input, 'grapefruit' );
- act( () => {
- input.blur();
- } );
+ await user.click( document.body );
+
expect( onChangeSpy ).toHaveBeenCalledTimes( 0 );
expectTokensNotToBeInTheDocument( [ 'grapefruit' ] );
@@ -235,17 +233,15 @@ describe( 'FormTokenField', () => {
// Add 'grapefruit' token by typing it and check blur of field tokenizes it.
await user.type( input, 'grapefruit' );
+ await user.click( document.body );
- act( () => {
- input.blur();
- } );
expect( onChangeSpy ).toHaveBeenNthCalledWith( 1, [
'grapefruit',
] );
expectTokensToBeInTheDocument( [ 'grapefruit' ] );
} );
- it( "should not add a token with the input's value when pressing the tab key", async () => {
+ it( "should not add a token with the input's value when tokenizeOnBlur is not set and pressing the tab key", async () => {
const user = userEvent.setup();
const onChangeSpy = jest.fn();
@@ -254,7 +250,6 @@ describe( 'FormTokenField', () => {
const input = screen.getByRole( 'combobox' );
- // Add 'orange' token by typing it and pressing enter to tokenize it.
await user.type( input, 'grapefruit' );
await user.tab();
expect( onChangeSpy ).toHaveBeenCalledTimes( 0 );
diff --git a/packages/components/src/form-token-field/types.ts b/packages/components/src/form-token-field/types.ts
index be34eadd1c..92e2d51fe1 100644
--- a/packages/components/src/form-token-field/types.ts
+++ b/packages/components/src/form-token-field/types.ts
@@ -166,7 +166,7 @@ export interface FormTokenFieldProps
*/
__next40pxDefaultSize?: boolean;
/**
- * If true, the select the first matching suggestion when the user presses
+ * If true, then select the first matching suggestion when the user presses
* the Enter key (or space when tokenizeOnSpace is true).
*
* @default false
@@ -183,7 +183,7 @@ export interface FormTokenFieldProps
*/
__nextHasNoMarginBottom?: boolean;
/**
- * If true add any incompleteTokenValue as a new token.
+ * If true, add any incompleteTokenValue as a new token when the field loses focus.
*
* @default false
*/
aaronrobertshaw
approved these changes
Sep 5, 2023
|
Merging this into #53835 first as it was branched off there, and we will do another full text of this functionality there before merging into trunk. Igoring php test failures as unrelated and should be fixed after rebase with trunk |
|
I personally prefer merging PRs separately especially when they affect more than one package, since they are easier to find/understand/revert if needed. But I understand if that was particularly tricky this time. Thank you anyway @glendaviesnz for the heads-up! |
Yeh, after rethinking this, the better approach will be to wait until the rest of the pattern category feature work is merged into trunk, and then merge this to trunk as a separate PR, so I have reverted that merge and added |
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?
Adds a
tokenizeOnBlurprop.Why?
To allow consuming components to specify that any
incompleteTokenValueis saved when the field loses focus. The main purpose for this is to prevent users saving the new add patterns model with unsaved categories added.How?
If
tokenizeOnBluristruethenaddNewTokenis run inonBlurTesting Instructions
Create patternmenu option'click outside the box so it loses focusScreenshots or screencast
Before:
form-token-before.mp4
After:
form-token-after.mp4