Repository navigation
DataForm: support validation in date field type - #72048
Conversation
jorgefilipecosta
added
[Feature] DataViews
|
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. |
| data={ data } | ||
| field={ field } | ||
| setValue={ setValue } |
There was a problem hiding this comment.
This is a bit too much. For example, setValue is already available via the field, and the value can be calculated via field.getValue as well. Can we improve this? Pass to this component (and the others) similar props as DataFormControlProps (embedding the onChange callbacks within), proper types (no any), etc.
| // Check HTML5 validity on all refs | ||
| const refs = Array.isArray( inputRefs ) ? inputRefs : [ inputRefs ]; | ||
| for ( const ref of refs ) { | ||
| const input = ref.current; | ||
| if ( input && ! input.validity.valid ) { | ||
| setCustomValidity( { | ||
| type: 'invalid', | ||
| message: input.validationMessage, | ||
| } ); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is essentially relaying the native error message for the input date (required) and use it as custom validation, so we can position it when we want (below the calendar). Nice :)
|
This is great work, thanks @jorgefilipecosta ! It's working nicely for me, the only bit I'd like us to improve before merging is this #72048 (comment) |
97d1887 to
fe844f7
Compare
oandregal
left a comment
oandregal
left a comment
There was a problem hiding this comment.
I've pushed the last adaptations in the internal controls. There are more things we can do (e.g., RelativeDateControl) but they're not essential to this PR. We can revisit later.
|
Flaky tests detected in 74cd149. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18344481155
|
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org> Co-authored-by: oandregal <oandregal@git.wordpress.org>
| { customValidity && ( | ||
| <p | ||
| className={ clsx( | ||
| 'components-validated-control__indicator', |
There was a problem hiding this comment.
I would prefer we not use external classes like this. It's very fragile and it will be strictly impossible once we move to CSS modules.
I have a question though — couldn't we just use the validation on the InputControl? Not doing it on the InputControl is actually detrimental because the input won't be semantically marked as invalid.
There was a problem hiding this comment.
Hi @mirka, thank you for your insights!
I have a question though — couldn't we just use the validation on the InputControl? Not doing it on the InputControl is actually detrimental because the input won't be semantically marked as invalid.
Unfortunately I tried that, and never managed to make it work, when I changed dates on the calendar outside the the InputControl the validation error did not appeared or cleared. I think this is an issue that we may try to solve the inputs only validate errors coming from a user actions and not show anything when the error comes from a value change outside.
In terms of UX, we would have an input, validation error in the middle and then the calendar which is part of the input, that also does not seems ideal.
InputControl is actually detrimental because the input won't be semantically marked as invalid.
That's a good point I guess we could pass an error prop to the input to fix this?
There was a problem hiding this comment.
I see! Then I think it's time to expose the components necessary for third-parties to build their own validated components. That was the plan to begin with, but we didn't have a use case yet. I can do some prep work first, and then we can try it on these date controls. How does that sound?
On the consumer side, I think the only prep work you will need is to wrap your compound date component into a single component that takes a label, onChange, and required prop. The system is supposed to work with any component that meets these requirements (theoretically 😄).
There was a problem hiding this comment.
Hi @mirka, That sounds perfect, thank you a lot for your help here
3 participants
This PR implements validation on the date field. It supports both html5 validation and custom validation for business rules similar to other fields.
The validation works for both when picking a single date or a range of dates.
A story was added allowing us to test how validation works.
Screenshot
Testing
Go to http://localhost:50240/?path=/story/dataviews-dataform--validation and test the date fields validation ( last 2 fields).
Notes for reviewers
Ignore whitespace otherwise it is harder to review https://github.com/WordPress/gutenberg/pull/72048/files?diff=unified&w=1.