CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
DataForm: Add a textarea control available for use with the text field type #71495
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 a textarea control available for use with the text field type #71495
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. |
598aa29
to
75a6b97
Compare
packages/dataviews/CHANGELOG.md
Outdated
|
||
- Introduce a new `DataViewsPicker` component. [#70971](https://github.com/WordPress/gutenberg/pull/70971) | ||
|
||
### Internal |
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 a nice public enhancement for the fields :)
packages/dataviews/src/types.ts
Outdated
/** | ||
* Arbitrary data to pass to the edit component. | ||
*/ | ||
editProps?: Record< string, any >; |
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 approach is problematic from two angles:
- It couples API and implementation (the specific component in use). If we do this, switching the internal component in use by DataForm to a different one (e.g., a new component library) is going to be impossible: the framework doesn't know which properties the consumer is using, and so it can't rematch them if some prop changes the name, for example — not to mention that the new component may not support certain properties the old one does.
- It competes with and dilutes the Field API. For example, why people would use
isValid.required
orisValid.pattern
if they just can passeditProps.required
directly? This is problematic because validation becomes something we can't analyze and use outside the component itself.
My suggestion is: can we add this control without the ability to modify it?
Unless we have a concrete use case where we need this immediately, that'd be my preference. It's easier to add new things later than to remove them. If we absolutely need it now for a specific use case, we could do:
{
type: 'text',
Edit: {
type: 'textarea',
rows: '3'
}
}
The Edit object would not allow any property, but only the ones we deem necessary. That way, we are in control and API-implementation are decoupled (and we can switch to a different component library, for example).
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 do like @oandregal's suggestion better, but I'm also not sure if it's strictly an Edit property, your remark about isRequired is good too.
In some cases, fields can have extra "properties" at the field level that would be good to pass to the form. For instance "min/max" this can be both a validation config and also something for "Edit". In other words even isValid.required might not be a "validation-only" concern and could be a top level property of the field.
All of this is also why I prefer this to be addressed separately from adding the control. It's its own concern.
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 tracking issue on Field validation may be relevant as context: #71500
I see min/max as part of isValid. And there's also isValid.elements
that has helped to absorb similar needs. For example, a case that came up was the ability to control the FormTokenField (array control): sometimes it allows users to add items not in the elements list, and sometimes it should not. The idea for this is to leverage isValid.elements
instead: if isValid.elements: true
the control won't allow users to introduce new values. This would also help in removing the logic we have in custom
for field types. See ongoing PR #71194
Another use case was adding prefixes/sufixes. See Automattic/wp-calypso#105213 (comment)
So, yeah, I agree we need something like this — though it needs to be controlled by the framework and looked at case by case to decide where it goes (isValid, Edit, etc.).
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.
Tracked this at #70936
Allow consumers to configure the bundled Edit control.
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.
All of this is also why I prefer this to be addressed separately from adding the control. It's its own concern.
Agreed. Thanks for the discussion here!
Overall I quite like the shape of what you've described, so it could even include resize
, too:
{
type: 'text',
Edit: {
type: 'textarea',
rows: '3',
resize: 'vertical'
}
}
But yes, let's explore this idea separately.
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 summarized this thread and created #71560 for shaping the API.
packages/dataviews/src/types.ts
Outdated
|
||
export type FieldType = | ||
| 'text' | ||
| 'textarea' |
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.
Can we add this as a control without exposing a specific FieldType for it? Consumers would use it this way:
{
type: 'text',
Edit: 'textarea'
}
The difference between a control and a FieldType is a blurry one. Field types communicate some kind of "domain" logic, they don't expose UI components nomenclature (e.g., we don't have a ToogleControl
field type but we do have a control for it). If we were to do this, we could perhaps have a longtext
field type. I may sound like a broken record, but I'd rather implement a longtext
when/if we need 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.
Agreed this should be just a control :)
id: 'alt_text', | ||
label: 'Alternative Text', | ||
type: 'textarea', | ||
editProps: { rows: 2 }, |
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'd prefer to leave this out of this PR and its own dedicated one.
75a6b97
to
8e47061
Compare
Thanks for the feedback @oandregal and @youknowriad! I've scaled this PR back to just add in the Slightly longer-term than this PR, but we'll also need to configure Let me know if you'd like to see any other changes! |
Flaky tests detected in 6b25b74. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17567674762
|
This is great, thanks @andrewserong Could you also update the DataForm validation story to include the textarea control? In the interest of timezones working to our advantage, I'm pre-approving but that'd be great to have. |
…ort setting a row value
…type, remove editProps
8e47061
to
6b25b74
Compare
Thanks again for the reviews! I've updated the validation story, and happy to continue on with improvements in follow-ups 👍 |
…d type (WordPress#71495) * DataForm: Try adding a textarea control with custom editProps to support setting a row value * Add changelog entries * Move changelog entries to the right place * Scale this back to just a separate control, but not a separate field type, remove editProps * Update validation story Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
What?
This PR proposes adding a
textarea
control to Dataviews/Dataform. In an earlier version of this PR, it also proposed making edit props configurable, as eventually customisation will be needed forrows
andresize
. For now, though, this PR just adds in the control.Why?
textarea
is a common requirement for form fieldsFor future PRs / exploration
textarea
also needs to support some level of customisation:resize
and set it to any of the valid CSS values (both
,horizontal
,vertical
,none
) — that isn't implemented in this PR, but it would be a good thing to keep in mind as we think about an API for custom valuesHow?
ValidatedTextareaControl
textarea
control as a valid control for use with thetext
field (i.e. usetext
as type and setEdit
totextarea
Questions
editProps
to allow configuration ofrows
. This has now been removed from the PR so that it can be discussed / explored separately to adding thetextarea
control.Testing Instructions
npm run storybook:dev
and navigate to the Default DataViews story. Scroll down to the bottom to see the text field for Description using a textarea control: https://localhost:50240/?path=/story/dataviews-dataform--defaultScreenshots or screencast
This screenshot shows two instances of the textarea field, with alternative text set to only display with 2 rows:
And the field type story for Text (now includes
textarea
):