-
Notifications
You must be signed in to change notification settings - Fork 4.5k
DataForm: add array
control
#71136
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
DataForm: add array
control
#71136
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @elazzabi! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
{ value: 'javascript', label: 'JavaScript' }, | ||
{ value: 'react', label: 'React' }, | ||
], | ||
__experimentalExpandOnFocus: true, |
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.
Does this need to be configurable? We can land this PR making this behavior the default in the component (without this prop) and only later revisit if we need to configure it.
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.
We (in Woo) need this to be expanded on focus (hence the prop here to try it out in Storybook). However, I'm not sure if it's the default behavior we want 😅
onChange, | ||
hideLabelFromVision, | ||
}: DataFormControlProps< Item > ) { | ||
const { id, label, placeholder, suggestions, ...extraProps } = field; |
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.
About ...extraProps
: if we need a field to configure a UI component, we may want to come with a structured (and safer) way to do it.
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.
The FormTokenField
component accepts many props, I'm not sure where we should draw the line of "You can change this, but you can't change that". Which is the reason why I used the ...extraProps
here. However, I'm open to have a structured object
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.
If back compat concerns are at play here, we should probably keep component-level customization to a minimum at this time, and also keep web standard terminology in mind when naming the properties in the public-facing API. The next-generation DS component that would handle this array type field will either be a combobox or a multiple select, but FormTokenField
props don't use web standard terminology for either pattern. We need to normalize the arbitrary/opinionated names into more standard ones (like the suggestions
→ elements
rename that was done in 7094494), that ideally will not be weird regardless of whether we end up with a combobox or multiple select.
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.
Perfect, that makes sense.
Keeping this in mind, I went ahead and added a few defaults here f48d7e0
- Make the selection open on focus, by default.
- Make sure the user can only select from the elements provided.
I think in the case of a multiselect, this makes sense as we want to see the possible options (open on focus), and we shouldn't be able to add new elements (ie, the validation function).
Let me know if that sounds good to you. I'll look into failing tests next
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.
Sounds good to me!
); | ||
|
||
// Custom validation function for FormTokenField | ||
const validateInput = useCallback( |
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.
What if the array doesn't provide any elements
? Users still need to be able to add values in that case:
Screen.Recording.2025-08-12.at.08.18.55.mov
We have a story for each field type. In the array story, can we add an example of field array without elements to make sure this case is testable?
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.
Additionally, if there are no elements provided, we could improve the user experience by:
- not displaying the suggestion list (
__experimentalExpandOnFocus
) - displaying the how to (
__experimentalShowHowTo
), so users know how to interact with the component
packages/dataviews/src/components/dataform/stories/index.story.tsx
Outdated
Show resolved
Hide resolved
@@ -51,7 +53,7 @@ export function getControl< Item >( | |||
return getControlByType( field.Edit ); | |||
} | |||
|
|||
if ( field.elements ) { |
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 is okay for now, it's how it's already working. I think a better approach would be to actually remove this from here and make every field definition absorb this (handle how they want to use or not use the elements
information). Leaving this as feedback in case you are available to follow up on this in a separate PR, as it involves testing/making sure every field type works well for editing and filtering.
Hi @elazzabi this is almost ready to merge, it only needs to address some edge cases. Thanks for continue to iterate on it. When you're ready, note that this also needs a changelog entry |
array
implementation to DataForm and update storybook implementationarray
control
packages/dataviews/src/components/dataform/stories/index.story.tsx
Outdated
Show resolved
Hide resolved
packages/dataviews/src/components/dataform/stories/index.story.tsx
Outdated
Show resolved
Hide resolved
packages/dataviews/src/components/dataform/stories/index.story.tsx
Outdated
Show resolved
Hide resolved
packages/dataviews/src/components/dataform/stories/index.story.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: André <[email protected]>
); | ||
|
||
const onChangeControl = useCallback( | ||
( tokens: ( string | { value: string } )[] ) => { |
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.
I'm trying to understand when the returned tokens can be an array of objects instead of strings. Unless I'm missing some context, I can't reproduce; every individual token is always a string. Can you confirm this? If so, can we simplify this to only handle strings?
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.
That is because the onChange
prop from FormTokenField
expects that. I haven't been able to reproduce this too, as it's always strings, but changing that will raise a TypeScript error 😞
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.
Ah, I missed that. It's all good, thanks for the context.
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.
Thanks for iterating on this one, Ahmed! I've left some minor comments, but this is working well otherwise. I'll merge when those are addressed.
I discussed with Ahmed a follow-up we want to make: there are use cases where we want to prevent users from introducing new items, if there are elements provided. This will be a follow-up PR. One way to make this work is by leveraging the validation mechanism: [
{
// Array field that allows users to introduce new values.
id: 'oneField',
type: 'array',
elements: [ /* ... */ ]
},
{
// Array field that doesn't allow users to introduce new values.
id: 'otherField',
type: 'array',
elements: [ /* ... */],
isValid: {
elements: true
}
}
] |
Congrats on your 1st contribution to Gutenberg, @elazzabi |
Thank you for the reviews and help @oandregal 🙏 |
This is a WIP
What?
This adds support for the
array
type in DataForm.Why?
This field can be used for tags, categories, and others.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
CleanShot.2025-08-08.at.11.51.00.mp4