DataForm: implement first prototype using duplicate page action #63032
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. |
oandregal
added
[Package] DataViews
packages/dataviews/src/dataform.tsx
Outdated
| }; | ||
|
|
||
| export default function DataForm< Item >( { | ||
| item: data, |
There was a problem hiding this comment.
According to #55101 (comment) there're use cases when DataForm could take an array of items as input.
There was a problem hiding this comment.
Makes me wonder whether we should always pass a single item though. Even though we're batch editing a list of items, we're still showing a single value per field. I wonder if there's a use-case where we show the multiple values somehow. Anyway, not that important for now.
|
Size Change: +4.37 kB (+0.25%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
This is what I got today. There're a few TODOs over the code that I'm going to look at in the coming days. Tomorrow, I'll focus on clarifying the last bit to have a generic |
packages/dataviews/src/dataform.tsx
Outdated
| const components = useMemo( | ||
| () => | ||
| fields.map( ( field ) => { | ||
| if ( field.type === 'text' ) { |
There was a problem hiding this comment.
Alternatively, instead of providing a type the field could offer an edit view to provide its own control. I found this to be an advance use case, so I decided to start with the type.
This is also a reason why starting with the actions is a good way to scope down the PR. The form rendered here needs to provide an input component, which we don't have in the Fields API yet — the render is only used in DataViews for now. When we get there, we need to consider whether the render can be shared between DataViews and DataForms. We already have situations where each type of view in DataViews needs different things, so it's not unthinkable that DataForms would need its own. I thought it'd be better to explore that problem space in a separate PR.
packages/dataviews/src/dataform.tsx
Outdated
| item: Item; | ||
| onUpdateItem: ( item: Item ) => void; | ||
| fields: NormalizedField< Item >[]; // TODO: use Field. Normalize them first. | ||
| form: { |
There was a problem hiding this comment.
Would metadata be a better name for this?
form seems to imply this is contains the actual form fields.
There was a problem hiding this comment.
As per #59745, I think we want to call it form, like we did in DataViews (e.g.: <DataViews view={view} />, <DataForm form={form} />).
There was a problem hiding this comment.
Actually for me the form object as I imagined it is less about these event handlers onChange, onSubmit... and more about the "layout" of the form, which fields are visible, how they are groups...
Seeing, what's currently in the form object, It raised the question about whether all the submit button and form submission handling should actually be part of DataForm or not.
I'm leaning towards no for the moment, for instance in the "details panel" case, there's no submit button. So I see DataForms as something you actually can use within a form but not the whole form. I see it as a group of *Control components basically. If we simplify, I see it as a component that has a value (data here) and a single onChange handler, all the rest is left to the wrapper component (submit...)
There was a problem hiding this comment.
I iterated on this and moved back the submit/cancel actions to the consumer.
I agree form is about the layout. If we wanted to absorb the form actions (submit/cancel) plus the corresponding labels, a better API could be:
<DataForm
data={ data }
fields={fields}
form={ form }
onUpdate={ onUpdate }
/* Absorb submit/cancel */
labels={ labels }
onSubmit={ onSubmit }
onCancel={ onCancel }
/>For an use case like the details panel, the interaction model suggested onUpdate would be triggered for every change. Though for modals like in this PR's use case, I went back and forth about triggering the update for every change vs only doing it upon submitting. The former seems a simpler approach, I didn't like we had to absorb the event handler for forms.
| @@ -39,6 +39,17 @@ import { CreateTemplatePartModalContents } from '../create-template-part-modal'; | |||
| const { PATTERN_TYPES, CreatePatternModalContents, useDuplicatePatternProps } = | |||
| unlock( patternsPrivateApis ); | |||
|
|
|||
| // TODO: this should be shared with other components (page-pages). | |||
There was a problem hiding this comment.
Consolidation can be done in a follow-up PR, but I wanted to gather early thoughts: do we create a registry for fields per postType? where do we extract the Field information for now? Actions are defined in the editor package while the edit-site package defines the DataViews.
There was a problem hiding this comment.
Yeah, I think right now the registry should be in the. editor/src/dataviews/fields folder like we have editor/src/dataviews/actions folder if we're talking about the default WP entities fields.
My thinking is that I'll probably move editor/src/dataviews to either a dedicated package @wordpress/core-dataviews or just move it into @wordpress/core-data because it's actually lower level than the editor and can be used outside the editor.
|
I want to look at improving a couple of TypeScript types (see TODOs), but other than that it's working and API ready. What this PR introduces: const form = {
visibleFields: [ ... ],
};
<DataForm
data={ data }
fields={ fields }
form={ form }
onUpdate={ onUpdate }
/>This PR assumes the In follow-up PRs we can expand the usage of
|
packages/dataviews/src/dataform.tsx
Outdated
| ); | ||
|
|
||
| return visibleFields.map( ( field ) => { | ||
| if ( field.type === 'text' ) { |
There was a problem hiding this comment.
It would be cool to model a "registry" (even if closed to start with) of field types.
const text = {
name: 'text',
edit: DataFormTextControl
}
This can materialize in follow-ups as we start expanding.
There was a problem hiding this comment.
Pushed changes to use a registry-like structure at a290321
oandregal
force-pushed
the
try/dataforms-for-duplicate-page
branch
2 times, most recently
from
c1582a2
to
d5b7c23
Compare
|
There are mobile test failures that don't seem related to this PR:
|
|
All feedback was addressed, the PR is working, and it's a good first step in terms of API & direction, in my view. Thoughts on landing this @youknowriad @jorgefilipecosta @ntsekouras? |
packages/dataviews/src/dataform.tsx
Outdated
| data: Item; | ||
| fields: Field< Item >[]; | ||
| form: Form; | ||
| onUpdate: Dispatch< SetStateAction< Item > >; |
There was a problem hiding this comment.
In DataViews, we have onChangeView and onSelectionChange and here we have onUpdate
I think I'd love for these to be:
- onChangeView
- onChangeSelection
- onChange (no suffix here to match Controls and because it's slightly different in terms of semantic, we're changing the data)
I'm also wondering whether data should be just value like the Controls component but that's more arguable.
There was a problem hiding this comment.
Renamed onUpdate to onChange at 3747020
data is in line with DataViews and also at some point we should work with multiple items, so I lean towards keeping it.
There was a problem hiding this comment.
#63087 renames onSelectionChange to onChangeSelection for dataviews.
| onUpdate: Dispatch< SetStateAction< Item > >; | ||
| }; | ||
|
|
||
| function DataFormTextControl< Item >( { |
There was a problem hiding this comment.
Would be cool to have a "types" folder with a file per type maybe at some point.
oandregal
force-pushed
the
try/dataforms-for-duplicate-page
branch
from
d5b7c23
to
3747020
Compare
|
Flaky tests detected in 3747020. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9777079856
|
|
This is currently blocked by mobile test failures that I don't see related to this PR:
Asked for help in the mobile slack channel (thread) to clarify what is happening. |
|
I've pushed 969ab6c as per @geriux suggestion in the mobile slack channel, which fixes the issue and unblocks this PR from landing. For full disclosure and future reference, this is what we know about what happened:
|
|
Should we add something about the new component to the changelog file? |
To the dataviews' changelog? I'm not sure. If this is supposed to become its own standalone package, I wouldn't add anything. I'd add it if this is a more permanent place where |
|
I'm not sure this one should be in a separate package. I think DataViews and DataForms will ultimately share a lot of things (field types ...) so I see this as a single package personally to render data in different shapes. |
|
Pushed a changelog entry at 8e06615 |
bph
added
[Type] Enhancement
…Press#63032) Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
I gave this a shot at #63849 |
Riad started working on this at #63840 |
Part of #59745
What?
First step in implementing the
DataFormAPI. It's only used by the duplicate action from pages, which is only available in the plugin.Why?
We want to start exploring this API for several aspects of our flows. The first step is in replacing the "Duplicate item" action for pages because:
How?
DataFormcomponent that temporarily lives within the@wordpress/dataviewsuntil we find a better place.Testing Instructions
Follow-ups
@wordpress/editorand@wordpress/router#63849