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
[Type] Code Quality
oandregal
force-pushed
the
simplify/field-api-normalization
branch
from
fd9364a to
47f458c
Compare
| * @return A field type definition. | ||
| */ | ||
| export default function getNormalizeFieldFunction< Item >( | ||
| function getDefaultProperties< Item >( |
There was a problem hiding this comment.
I don't understand what this function is about?
There was a problem hiding this comment.
I guess your question is why not to inline this within normalizeFields, right?
I thought it'd be best that it stays as a separate function because this acts like the public registry we don't have. So when we have the registry, it'd be a simple call switch.
There was a problem hiding this comment.
No, I'm actually not sure what this function is about. Is it some kind of getFieldTypeByName or something? If that's the case, I kind of expected this to loop through the fields and compare "id" or something. The if/else or switch feels a bit weird.
There was a problem hiding this comment.
This function just loads the defaults defined by each type in the corresponding file. I guess getFieldTypeByName could work as a name as well.
There was a problem hiding this comment.
Should it be something like
[ string, number ... ].find( fieldType => fieldType.type === type )
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
I find this a lot clearer. Thanks for the follow-up
oandregal
force-pushed
the
simplify/field-api-normalization
branch
from
eeb173c to
d472229
Compare
| | 'over'; | ||
|
|
||
| export type FieldType = | ||
| export type FieldTypeName = |
There was a problem hiding this comment.
@youknowriad I lost the comment attached to the commit, so continuing here: perhaps FieldTypeName is a good fit for this one.
There was a problem hiding this comment.
Works for me, although "string" would have worked for me assuming we'll allow registration at some point.
2 participants
Follow-up to #73387
Related #73548
What?
After #73387 each field type provided a normalize function that effectively acted as a factory for the given field type. This changes it back to return an object with the default properties.
Why?
See feedback #73387
Testing Instructions
Smoke test DataViews storybook and the Site Editor (Pages, Templates, Patterns screens).