-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
DataViews: make view.hiddenFields
optional
#62876
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. |
Size Change: +6 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
@@ -216,7 +216,7 @@ interface ViewBase { | |||
/** | |||
* The hidden fields. | |||
*/ | |||
hiddenFields: string[]; | |||
hiddenFields?: string[]; |
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've been reading our types lately and this one actually stood out to me :P
I think it makes more sense to have "visibleFields" rather than "hiddenFields" as we move forward. I'm pretty sure most entities will have a lot more fields that what we currently have and an allow list would be better.
Omitting it could have the same effect as today when you omit hiddenFields.
Not a concern for this PR though, just a thought that I had.
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.
Yeah, that's a good point: post types have a lot of metadata and if we aim to share the field definition between a view and a form (e.g.: details panel), a lot of the fields would be hidden in the view.
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.
LGTM 👍
What?
This PR makes the
view.hiddenFields
config optional.Why?
It should not be required to provide an empty
hiddenFields
array it the consumer doesn't have any hidden fields.How?
hiddenFields
type to optional.visibleFields
.DEFAULT_VIEW
config.Testing Instructions
Visit "Site Editor > Patterns" and verify everything works as expected.
Before this PR, if the
hiddenFields
property was removed from here (468c294) the Patterns page would not load.