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: Fix SelectControl size and spacing #64324

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

youknowriad
Copy link
Contributor

Related #59745

What?

Small PR to improve the consistency of the size and spacing of the controls that are generated by the DataForm component. Specifically this fixes the size and spacing for the "select" control for string fields.

Before
Screenshot 2024-08-07 at 12 45 46 PM

After
Screenshot 2024-08-07 at 12 48 56 PM

Check the size and spacing of the status control.

@youknowriad youknowriad added [Type] Enhancement A suggestion for improvement. [Feature] Data Views 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 <youknowriad@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: oandregal <oandregal@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>

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

@youknowriad
Copy link
Contributor Author

The other small thing I'm noticing in the screenshots above is that the padding left of the first control (TextControl) is different than the other controls (NumberControl and SelectControl). I think this comes directly from the underlying components. So I'm wondering if you folks are aware of it cc @tyxla @mirka

@t-hamano
Copy link
Contributor

t-hamano commented Aug 7, 2024

the padding left of the first control (TextControl) is different than the other controls (NumberControl and SelectControl).

This issue has been reported in #57394

Comment on lines +70 to +71
__next40pxDefaultSize
__nextHasNoMarginBottom
Copy link
Member

Choose a reason for hiding this comment

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

No problem if this PR merges first, but FYI these props are being added in bulk to all in-app SelectControls in #64283.

@youknowriad youknowriad enabled auto-merge (squash) August 7, 2024 11:25
@youknowriad youknowriad merged commit e6627e0 into trunk Aug 7, 2024
69 of 70 checks passed
@youknowriad youknowriad deleted the fix/data-form-select-size branch August 7, 2024 11:25
@github-actions github-actions bot added this to the Gutenberg 19.0 milestone Aug 7, 2024
@Zealth57
Copy link

Zealth57 commented Aug 8, 2024

Not sure if this requires a separate Github issue or it can be discussed in this thread here... Looking through the docs in data form, the form elements seem like they could be made a little more intuitive. https://wordpress.github.io/gutenberg/?path=/docs/dataviews-dataform--docs. It looks like when trying to generate a dropdown, you pass the 'type' in as 'text' and then it creates a dropdown. It's not clear how to create other elements that use multiple options such as radio and checkboxes. I think the 'type' should match what you want to output, in the case of the documented examples the 'type' for author and status should be something like 'select'. If you wanted the status to be radio you'd do something like 'radio'.

We should also consider accessibility with the more advanced types such as checkboxes and radios. There should be additional properties for things like 'legendLabel' so things can be wrapped in valid markup with a legend.

Lastly, for 'type' of 'integer', I wonder if 'number' is more appropriate since that's the actual type in html5 so as a consumer of the component I'd naturally think of 'number' as the value and not 'integer'.

@youknowriad
Copy link
Contributor Author

youknowriad commented Aug 8, 2024

It looks like when trying to generate a dropdown, you pass the 'type' in as 'text' and then it creates a dropdown. It's not clear how to create other elements that use multiple options such as radio and checkboxes. I think the 'type' should match what you want to output, in the case of the documented examples the 'type' for author and status should be something like 'select'. If you wanted the status to be radio you'd do something like 'radio'.

We definitely didn't invest much in docs yet as it's bit early but the idea is that if you pass elements array in the field definition, it becomes a select be it a "text" or an "integer.

If you wanted the status to be radio you'd do something like 'radio'.

Obviously, we'd need this kind of customization but we're starting very small and iterating/adding controls types (radio, toggles, ...) as we implement UIs in Core (quick edit focus for now).

We should also consider accessibility with the more advanced types such as checkboxes and radios. There should be additional properties for things like 'legendLabel' so things can be wrapped in valid markup with a legend.

I agree that we need ways to "group" fields... I've heard a few things already:

  • fieldsets
  • tabs
  • ...

We also need support for things like "description", "help" for fields.

Lastly, for 'type' of 'integer', I wonder if 'number' is more appropriate since that's the actual type in html5 so as a consumer of the component I'd naturally think of 'number' as the value and not 'integer'.

We discussed this with @oandregal already, I think it's not settled but André had some good arguments.

@oandregal
Copy link
Member

oandregal commented Aug 8, 2024

Lastly, for 'type' of 'integer', I wonder if 'number' is more appropriate since that's the actual type in html5 so as a consumer of the component I'd naturally think of 'number' as the value and not 'integer'.

The type field is used for various things: sorting, validation, and also rendering the control for edit the field. Text and integer work well for all those functions while radio wouldn't. Integers and doubles are also different in terms of validation, for example. The current types aim to match the data type (e.g.: like in the REST API) and not HTML controls/specs.

Of course, we've just started. We will need to build more nuances to decide whether to render a radio or a select, how to group fields together, etc.

@Zealth57
Copy link

Zealth57 commented Aug 8, 2024

The REST types are the way they are because they’re independent of form data. You can send data derived from any source to REST and have it validated which is why “Type” works well in that context. When defining “Type” in DataForm, it’s confusing because html form elements have the “type” attribute. Also it’s not intuitive or obvious what input will be output when I put in “type” of “integer”. What if I want a drop down of elements and validate all as integer? I think validation, sorting, all that is totally separate from “Type”. Maybe we should do “validateAs” as a separate property and that’s what gets “integer”.

I know this is early and documentation isn’t done but that doesn’t mean we can’t properly account for future elements. Using type “select” for drop downs now helps make this easier to use and starts to give a pattern for the future. Making text output select now seems like we will have to either make a breaking change later or have a not very intuitive experience as a dev. Documentation is nice but even better is when documentation is not required because something is so easy and intuitive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Data Views 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.

5 participants