-
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
Merged
oandregal
merged 28 commits into
WordPress:trunk
from
dinhtungdu:feat/dataview-number-field
Sep 30, 2025
Merged
DataViews: Add number field
#71797
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
dd82d31
chore: scaffold
dinhtungdu a3b78ca
feat: update field type
dinhtungdu 2ab54e8
feat: update field control
dinhtungdu 161d167
feat: add step support
dinhtungdu f76354e
feat: update validation
dinhtungdu 754a11b
feat: register new field
dinhtungdu 6b78a80
feat: add suffix and prefix support
dinhtungdu 3000524
feat: update min max of between control
dinhtungdu 429a54b
fix: update custom validation trigger condition
dinhtungdu 209f2cd
test: add number tests
dinhtungdu b8cb310
fix: between number onChange callback
dinhtungdu 5b37b13
fix: Ensure the value of min max are always different
dinhtungdu 5f3d926
fix: type consistency
dinhtungdu ce9c026
fix: number validation logic
dinhtungdu 483510e
dev: story
dinhtungdu 7f81c2b
refactor: base integer on number
dinhtungdu 8e20976
docs: update README file
dinhtungdu b605910
chore: changelog
dinhtungdu 74acb65
chore: restore integer field definition
dinhtungdu 8ce0f13
fix: simplify the number field for the first iteration, remove steps/…
dinhtungdu 9cfddf9
fix: remove unused helper
dinhtungdu d2c03b7
fix: remove unused types related to number
dinhtungdu 0716841
fix: make the render function respect default decimal number
dinhtungdu 1ccff00
fix: unify validation behavior
dinhtungdu 93c776c
refactor: extract number control into a shared component for number a…
dinhtungdu 157b176
fix: sync the validation with integer
dinhtungdu 17aab3a
Update story components to prevent conflicts with global namespace
oandregal 41af593
fix: update number with element story
dinhtungdu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,185 +1,9 @@ | ||
| /** | ||
| * External dependencies | ||
| */ | ||
| import deepMerge from 'deepmerge'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { | ||
| Flex, | ||
| BaseControl, | ||
| __experimentalNumberControl as NumberControl, | ||
| privateApis, | ||
| } from '@wordpress/components'; | ||
| import { useCallback, useState } from '@wordpress/element'; | ||
| import { __ } from '@wordpress/i18n'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { OPERATOR_BETWEEN } from '../constants'; | ||
| import type { DataFormControlProps } from '../types'; | ||
| import { unlock } from '../lock-unlock'; | ||
|
|
||
| const { ValidatedNumberControl } = unlock( privateApis ); | ||
|
|
||
| type IntegerBetween = [ number | string, number | string ]; | ||
|
|
||
| function BetweenControls( { | ||
| value, | ||
| onChange, | ||
| hideLabelFromVision, | ||
| }: { | ||
| value: IntegerBetween; | ||
| onChange: ( [ min, max ]: IntegerBetween ) => void; | ||
| hideLabelFromVision?: boolean; | ||
| } ) { | ||
| const [ min = '', max = '' ] = value; | ||
|
|
||
| const onChangeMin = useCallback( | ||
| ( newValue: string | undefined ) => | ||
| onChange( [ Number( newValue ), max ] ), | ||
| [ onChange, max ] | ||
| ); | ||
|
|
||
| const onChangeMax = useCallback( | ||
| ( newValue: string | undefined ) => | ||
| onChange( [ min, Number( newValue ) ] ), | ||
| [ onChange, min ] | ||
| ); | ||
|
|
||
| return ( | ||
| <BaseControl | ||
| __nextHasNoMarginBottom | ||
| help={ __( 'The max. value must be greater than the min. value.' ) } | ||
| > | ||
| <Flex direction="row" gap={ 4 }> | ||
| <NumberControl | ||
| label={ __( 'Min.' ) } | ||
| value={ min } | ||
| max={ max ? Number( max ) - 1 : undefined } | ||
| onChange={ onChangeMin } | ||
| __next40pxDefaultSize | ||
| hideLabelFromVision={ hideLabelFromVision } | ||
| /> | ||
| <NumberControl | ||
| label={ __( 'Max.' ) } | ||
| value={ max } | ||
| min={ min ? Number( min ) + 1 : undefined } | ||
| onChange={ onChangeMax } | ||
| __next40pxDefaultSize | ||
| hideLabelFromVision={ hideLabelFromVision } | ||
| /> | ||
| </Flex> | ||
| </BaseControl> | ||
| ); | ||
| } | ||
|
|
||
| export default function Integer< Item >( { | ||
| data, | ||
| field, | ||
| onChange, | ||
| hideLabelFromVision, | ||
| operator, | ||
| }: DataFormControlProps< Item > ) { | ||
| const { label, description, getValue, setValue } = field; | ||
| const value = getValue( { item: data } ) ?? ''; | ||
| const [ customValidity, setCustomValidity ] = | ||
| useState< | ||
| React.ComponentProps< | ||
| typeof ValidatedNumberControl | ||
| >[ 'customValidity' ] | ||
| >( undefined ); | ||
|
|
||
| const onChangeControl = useCallback( | ||
| ( newValue: string | undefined ) => { | ||
| onChange( | ||
| setValue( { | ||
| item: data, | ||
| // Do not convert an empty string or undefined to a number, | ||
| // otherwise there's a mismatch between the UI control (empty) | ||
| // and the data relied by onChange (0). | ||
| value: [ '', undefined ].includes( newValue ) | ||
| ? undefined | ||
| : Number( newValue ), | ||
| } ) | ||
| ); | ||
| }, | ||
| [ data, onChange, setValue ] | ||
| ); | ||
|
|
||
| const onChangeBetweenControls = useCallback( | ||
| ( newValue: IntegerBetween ) => { | ||
| onChange( | ||
| setValue( { | ||
| item: data, | ||
| value: newValue, | ||
| } ) | ||
| ); | ||
| }, | ||
| [ data, onChange, setValue ] | ||
| ); | ||
|
|
||
| const onValidateControl = useCallback( | ||
| ( newValue: any ) => { | ||
| const message = field.isValid?.custom?.( | ||
| deepMerge( | ||
| data, | ||
| setValue( { | ||
| item: data, | ||
| value: [ undefined, '', null ].includes( newValue ) | ||
| ? undefined | ||
| : Number( newValue ), | ||
| } ) as Partial< Item > | ||
| ), | ||
| field | ||
| ); | ||
|
|
||
| if ( message ) { | ||
| setCustomValidity( { | ||
| type: 'invalid', | ||
| message, | ||
| } ); | ||
| return; | ||
| } | ||
|
|
||
| setCustomValidity( undefined ); | ||
| }, | ||
| [ data, field, setValue ] | ||
| ); | ||
|
|
||
| if ( operator === OPERATOR_BETWEEN ) { | ||
| let valueBetween: IntegerBetween = [ '', '' ]; | ||
| if ( | ||
| Array.isArray( value ) && | ||
| value.length === 2 && | ||
| value.every( | ||
| ( element ) => typeof element === 'number' || element === '' | ||
| ) | ||
| ) { | ||
| valueBetween = value as IntegerBetween; | ||
| } | ||
| return ( | ||
| <BetweenControls | ||
| value={ valueBetween } | ||
| onChange={ onChangeBetweenControls } | ||
| hideLabelFromVision={ hideLabelFromVision } | ||
| /> | ||
| ); | ||
| } | ||
| import ValidatedNumber from './utils/validated-number'; | ||
|
|
||
| return ( | ||
| <ValidatedNumberControl | ||
| required={ !! field.isValid?.required } | ||
| onValidate={ onValidateControl } | ||
| customValidity={ customValidity } | ||
| label={ label } | ||
| help={ description } | ||
| value={ value } | ||
| onChange={ onChangeControl } | ||
| __next40pxDefaultSize | ||
| hideLabelFromVision={ hideLabelFromVision } | ||
| /> | ||
| ); | ||
| export default function Number< Item >( props: DataFormControlProps< Item > ) { | ||
| return <ValidatedNumber { ...props } decimals={ 0 } />; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import type { DataFormControlProps } from '../types'; | ||
| import ValidatedNumber from './utils/validated-number'; | ||
|
|
||
| export default function Number< Item >( props: DataFormControlProps< Item > ) { | ||
| // TODO: remove this hardcoded value when the decimal number is configurable | ||
| return <ValidatedNumber { ...props } decimals={ 2 } />; | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: decimalandtype: integer. This is how I envision the two of them working together and evolving:type: integerrender: an integerEdit: 'integer'by default (uses ValidatedNumberControl under the hood)prefixsuffixseparatorThousandisValid: checks validity for integertype: decimalrender: a decimal number with two decimals by defaultEdit: 'decimal'by default (uses ValidatedNumberControl under the hood with step set at 0.01)prefixsuffixdecimals(e.g.,decimals: 2would setstep=0.01under the hood)separatorThousandseparatorDecimalisValid: checks validity for the decimal numberI think we should cover all use cases. We don't introduce
separatorThousandandseparatorDecimalin this PR (this is powered by a new Edit component, separate fromintegerandnumber). 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
decimalsprop for Edit config than astepprop that mimics the component/browser's.decimalsis clearer thanstep.decimalsworks better as a prop to configure therenderfunction too, wherestepdoesn't make sense, if/when we do that (e.g.,render: { decimals: 2 }). We can introducesteplater 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 = 0it becomes an integer field, I'm not really convinced that these should be separate.Uh oh!
There was an error while loading. Please reload this page.
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.
But isn't
numbermore common thandecimal? JSON Schema and Open API define two numeric types:numberandinteger. As we're shaping the API, I think we should also consider the familiarity of a term over the other.It depends on which level
decimalsbelong to? In this convo context,decimalsis an EditConfiguration property, which affects only the Edit component. Then validation is our problem.If
decimalsis a field property, then yes,decimals = 0becomes 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:
integerandnumber/decimal. The field configuration also looks simple forintegerfields; consumers just need to specify the field type. Having to providedecimals: 0to declareintegermakes 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,decimalsand 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.
This one feels wrong to me.
childrencould be just a regularfieldno? so{ type: 'number', format: { decimals: 0 }The other thought I have is that I'm not sure
formatis 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
EditConfigurationtoformat: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.
We shouldn't do this. The existing
EditConfigare 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 (
numberas 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.