Skip to content
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: Use the fields array to define the order of the fields #64335

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

youknowriad
Copy link
Contributor

Related #59745

What?

This PR updates the DataForm component to use the form.fields array order to render the fields. Instead of relying on the order of the fields in the definitions.

Testing Instructions

  • Open the storybook DataForm story.
  • Change the order of the fields in the form const
  • Notice that it impacts the output of the form.

@youknowriad youknowriad added [Type] Enhancement A suggestion for improvement. [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond labels Aug 7, 2024
@youknowriad youknowriad self-assigned this Aug 7, 2024
Copy link

github-actions bot commented Aug 7, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: youknowriad <[email protected]>
Co-authored-by: ntsekouras <[email protected]>
Co-authored-by: Zealth57 <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Zealth57
Copy link

Zealth57 commented Aug 8, 2024

It seems like having separate properties for form elements and the order of the elements introduces extra complexity on the code and on the user trying to use the component. Is there a reason we can't just use the order from the fields property? Why would they need to be created in a different order in one place and output in a separate order in another?

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@ntsekouras
Copy link
Contributor

Why would they need to be created in a different order in one place and output in a separate order in another?

They are different contexts (view/edit) so having control over this is more flexible, no?

@youknowriad
Copy link
Contributor Author

It seems like having separate properties for form elements and the order of the elements introduces extra complexity on the code and on the user trying to use the component. Is there a reason we can't just use the order from the fields property? Why would they need to be created in a different order in one place and output in a separate order in another?

The fields properties is like a library of fields, we won't be showing all the fields of a post type in the form or in the list view, each form or list view need to pick and organize which fields to render. So I think that's a "form" object property. For DataViews for instance, it's something that the user can actually change in the UI (without touching the fields library)

@youknowriad youknowriad merged commit d578bb2 into trunk Aug 8, 2024
69 of 70 checks passed
@youknowriad youknowriad deleted the update/dataform-field-order branch August 8, 2024 07:14
@github-actions github-actions bot added this to the Gutenberg 19.1 milestone Aug 8, 2024
@oandregal
Copy link
Member

Can we do the same for DataViews to stay consistent? Even if the user can change the order, it's confusing from a developer point of view (also users) that the order in DataViews is determined by the fields array and the order in DataForm comes from the form.fields field.

@youknowriad
Copy link
Contributor Author

Can we do the same for DataViews to stay consistent?

Isn't it already the case? Since you can reorder the columns in table, I think it's already the case.

@Zealth57
Copy link

Zealth57 commented Aug 8, 2024

There’s 2 separate things: there’s a user trying to define a form and then there is manipulation to the form. What both of you described is manipulation to the form via user input or some other component manipulating the order. If something manipulates the order it should just modify the 1 property and pass it back in with the new order it needs. When I define a form as a user interacting with this component I don’t see why I would ever define the order differently from defining the fields. Make the component trying to manipulate it handle the order, not the user. At bare minimum the default should be use the order defined in fields, and the separate property is optional.

@youknowriad
Copy link
Contributor Author

When I define a form as a user interacting with this component I don’t see why I would ever define the order differently from defining the fields.

I think what you're missing is that the "fields" definition is not specific to a form. In WordPress the same "fields" will be defined for a post type for both rendering forms for that post type but also rendering lists for that post types (and probably more). So It's important to decouple the fields registration/library from the form definition.

@Zealth57
Copy link

Zealth57 commented Aug 8, 2024

I’m not suggesting we couple field registration/library from the definition. I’m saying that passing in the fields in one property but defining the “order” of the fields in another seems confusing and unnecessary in most use cases.

if a higher component using DataForm can create the fields and know the order it needs, define the fields in that order from the start. We can’t say that these post types can define fields but aren’t capable of defining order.

Separate from all that, as someone who wants to use DataForm separately from posts, if I fully control the fields definition then the order is redundant. I still see big value in making the ordering property optional, and simply use what’s provided in fields first.

@youknowriad
Copy link
Contributor Author

@Zealth57 I understand but I think it doesn't really scale well to when we introduce things like "tabs", "groups"... because at that point the form property becomes more something like:

const form = { tabs: [ { name: "First", fields: [ "a', "b" ] },  { name: "First", fields: [ "c", "d" ] } ] }

At this point there's "no single global order" and I believe bundling the "fields" definition within the "tabs" definition is very bad because you may want to refer to fields elsewhere (outside the "tabs" property).

I think having a default when you omit "fields" from form is ok and we can add support for it. We can say render all the fields as they appear in the definition but that's an edge case.

@Zealth57
Copy link

Zealth57 commented Aug 8, 2024

I agree that when we need to support more advanced forms with tabs that a separate property will be necessary and easier to use for sure. Especially with grouping tabs or panels etc.

I disagree it’s an edge case though, thinking as a developer trying to create a plugin and use this package (which is what prompted my original comment actually is when I tried to use this). Developers need to create settings pages natively and not use old php. I’m creating a configuration page much like Settings -> General in the admin today. I define the fields for the user and want a nice standard form output. No manipulation, changes, etc. Core will have many settings forms that are very straight forward and the shape of fields is the order. So while I appreciate the flexibility and see value in the property, as a user of the package it makes it more work and complexity to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants