CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
DataViews: Add number
field
#71797
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: Add number
field
#71797
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. |
telephone, | ||
url, | ||
integer, | ||
number, |
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.
Should we remove "integer"? Can we make "number" generic enough to handle all the cases. What's the props and cons of each approach?
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.
We need to consider both the field type and the Edit function as separate entities. Introducing new field types is only justified if they provide value in more places than Edit
, otherwise, it's just Edit configuration.
Everything becomes a bit more clear if we go with type: decimal
and type: integer
. This is how I envision the two of them working together and evolving:
type: integer
render
: an integerEdit: 'integer'
by default (uses ValidatedNumberControl under the hood)- Edit config:
prefix
suffix
separatorThousand
isValid
: checks validity for integer
type: decimal
render
: a decimal number with two decimals by defaultEdit: 'decimal'
by default (uses ValidatedNumberControl under the hood with step set at 0.01)- Edit config
prefix
suffix
decimals
(e.g.,decimals: 2
would setstep=0.01
under the hood)separatorThousand
separatorDecimal
isValid
: checks validity for the decimal number
I think we should cover all use cases. We don't introduce separatorThousand
and separatorDecimal
in this PR (this is powered by a new Edit component, separate from integer
and number
). I think we could challenge a bit this idea and, if they are really necessary, we can introduce them later, in a follow-up PR.
I'd rather expose a decimals
prop for Edit config than a step
prop that mimics the component/browser's. decimals
is clearer than step
. decimals
works better as a prop to configure the render
function too, where step
doesn't make sense, if/when we do that (e.g., render: { decimals: 2 }
). We can introduce step
later as something specific to Edit, if it becomes necessary. Happy to discuss this and be convinced otherwise if anyone has ever used steps in practice and has feedback to provide.
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.
If decimals = 0
it becomes an integer field, I'm not really convinced that these should be separate.
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.
Everything becomes a bit more clear if we go with
type: decimal
andtype: integer
.
But isn't number
more common than decimal
? JSON Schema and Open API define two numeric types: number
and integer
. As we're shaping the API, I think we should also consider the familiarity of a term over the other.
If decimals = 0 it becomes an integer field,
It depends on which level decimals
belong to? In this convo context, decimals
is an EditConfiguration property, which affects only the Edit component. Then validation is our problem.
If decimals
is a field property, then yes, decimals = 0
becomes an integer, but it doesn't make sense to add it as a field level, because only "number" needs it.
With current API design, we have to expose two field types: integer
and number
/decimal
. The field configuration also looks simple for integer
fields; consumers just need to specify the field type. Having to provide decimals: 0
to declare integer
makes it look too verbose to me.
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 always thought min
, max
, decimals
and things like that are not just about validation or "edit config" but more about config for the field type. I know it's not the case today though.
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.
children: {
elementType: 'number',
decimals: 0,
}
This one feels wrong to me. children
could be just a regular field
no? so { type: 'number', format: { decimals: 0 }
The other thought I have is that I'm not sure format
is a "wide" enough term that covers all the config that we need but I also don't have strong counter examples at the moment.
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.
JSONSchema has both "number" and "integer" as well. So that's a strong argument for me. I invite you to also check how JSON schema define all these formats... I think the closer we get to JSONSchema, the better. It makes our fields even more portable and usable with other tools.
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.
Sounds like we should migrate the whole EditConfiguration
to format
:D, let me do it and see how it looks.
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.
should migrate the whole EditConfiguration to format
We shouldn't do this. The existing EditConfig
are actually bound to the edit components: rows for textarea, prefix/suffix for text (e.g., adding the @
prefix for email fields, etc.). That's not a format, but edit configuration and still needs to live as such.
My suggestion would be to scope this PR to just introducing a new number type (number
as per your suggestions) — that's something we've got agreement in this thread. Do not add any support for prefix/suffix/steps yet, just make it use two decimals for now. In a follow-up, we'll consolidate the way forward for formatting.
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.
Makese sense! I updated the PR to only adding the number field, can you please take a look.
@@ -0,0 +1,217 @@ | |||
/** |
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 think it'd be better to create a utils/validated-number.tsx
utility (see existing utils/validated-input.tsx
) that it's used by both integer and decimal controls.
Note these controls are used in both Edit and the filters, so we need to test everything there work properly.
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.
As we're just adding the number field for now, I would wait untile we consolidate the integer and number fields to create the utility. WDYT?
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.
Sorry, not sure I understand what do we need to consolidate. What I see is that the number control is a copy of the integer with two tweaks: the steps + the toNumberOrEmpty
function (that would also be good to have in integer, I guess).
Not a blocker for landing, but it'd be best to have a single implementation for numbers that both integer and number use.
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 meant that I want to wait until we refactor the integer field to extract the control into a ValidatedNumber
component. Probably a follow-up PR right after this.
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 changed my mind, the change is pretty minimal.
Thanks, I agree it's a good call. I suppose you're aware of #71500, but |
if ( isEmpty( value ) ) { | ||
return null; | ||
} |
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.
Unlike any other field type, a number would be valid if it is empty in this default custom function, even if it provides a list of elements.
I think we could improve this for all field types by leveraging the new isValid.elements
validation (only check for elements if this flag is true), but, for now, I'd rather have a consistent behaviour across fields.
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.
even if it provides a list of elements.
I think if the field is not required, then an empty value is valid, no? When the field is required, the required validation will catch it first, so that check will always return false. But having a consistent behavior across fields makes sense, so let me unify the behavior for now.
const value = field.getValue( { item } ); | ||
if ( ! isEmpty( value ) && field.elements ) { | ||
const numericValue = Number( value ); | ||
const match = field.elements.find( | ||
( element ) => | ||
Number.isFinite( Number( element.value ) ) && | ||
Number( element.value ) === numericValue | ||
); | ||
if ( match ) { | ||
return match.label; | ||
} | ||
} | ||
return value; |
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.
This just renders the value, and 0 to the right are ignored. I guess this is fine for now. However, when introducing a way to set N decimals, the default render should also consider the decimals in render (Number.toFixed( N )
).
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.
As we're defaulting to 2 decimal numbers, let me hard-code it for now.
@dinhtungdu could you also update the validation story by adding a number next to the integer example? Other than that, rebasing, and the minor implementation things, this is ready to land, so I'm pre-approving. |
f95826c
to
dfa510e
Compare
dfa510e
to
157b176
Compare
What?
This PR adds a
number
field to DataForm/DataView. This also refactors theinteger
field to utilize the new number field internally.Why?
It's common to handle decimal numbers, so DataViews/DataForm should provide a
number
field.How?
number
field frominteger
field.Add step, prefix, and suffix support.Fix the validation issue with integer field.Refactor theinteger
field based on the newnumber
field by setting thestep
to1
.Notes:
I'm on the fence about addingAs discussed below, we decided to just create the new number field with default step set tomin
andmax
, but I decided to skip it in this first iteration.0.01
. We will review the API and consolidate the integer and number field in follow-up PRs.Testing Instructions
Check the storybook for
number
andinteger
fields, see field rendering, editing, and filtering work as expected.