DataForm: add description support for the combined fields and show the description in the Card layout #71380
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. |
| // If it doesn't have a header, keep it open. | ||
| // Otherwise, the card will not be visible. | ||
| <CardBody className="dataforms-layouts-card__field-control"> | ||
| { field.description && ( |
There was a problem hiding this comment.
This works for combined form fields (form fields with children), which is the main use case.
What about form fields that don't have children? In the LayoutCard story, there's an example with a single-form-field card, the payments card. Adding a label or description to that form field is ignored because it doesn't have children. It uses the label and description coming from the field definition, but note the description is attached to the regular control:
| with header | no header |
|---|---|
 |
 |
It's perhaps fine to address only combined form fields in this PR. We can revisit single form fields in a follow-up.
There was a problem hiding this comment.
I mentioned this in the PR description. Single fields using the Card layout are still rendered using the Regular layout. The field description is already handled by its Edit component, so I don't think the Card layout for a single field should show the description.
@oandregal It's not a big lift to update the single field case, but should we? The field can have two descriptions; could that be an issue?
There was a problem hiding this comment.
It's fine as it is. My thinking is that "simple fields" may want to have a label and description, like combined field have. But that's a separate discussion we can address when/if that's raised.
|
@dinhtungdu this PR needs a changelog entry (see CI error). Follow the format that exists in |
oandregal
added
[Type] Enhancement
packages/core-data/CHANGELOG.md
Outdated
|
|
||
| ## Unreleased | ||
|
|
||
| ### Enhancements |
There was a problem hiding this comment.
This should be in packages/DATAVIEWS/changelog, not in core-data :) Also, it looks like your editor adds some formatting to other areas of the changelog.
There was a problem hiding this comment.
Oh my bad, I fixed it in the latest commit. 😅
dinhtungdu
force-pushed
the
feat/card-layout-description
branch
from
703a7be to
35f453a
Compare
|
@oandregal Can you help me merge this PR? I don't have permission to do it. Btw, the failed E2E test seems irrelevant. |
Sure, happy to. I'm seeing some weird things that I don't know how to fix: the checks for node.js and PHP are expected/waiting for hours now. I asked in WordPress slack core-editor for advice. In lieu of that, my recommendation would be to rebase this PR from trunk. |
It turns out this would solve the problem :) There were some actions changes a few days ago and this PR is older than that. |
dinhtungdu
force-pushed
the
feat/card-layout-description
branch
from
35f453a to
40c6d9e
Compare
2 participants
What?
This PR adds description support for the combined fields and shows that description in the Card layout
Why?
For the combined field, we might also want to inform users of the purpose of the section. This is why we added the new description property here. In this PR, we are only showing that description for the Card layout because it should be handled at the layout level, as each layout has different needs and positions to display the description.
How?
description?: stringproperty toCombinedFormFieldtypeTesting Instructions
Screenshots or screencast