fix: correctly validate required TimePicker

[bitpshr]

Type: bug

The following has been addressed in the PR:

• There is a related issue
• All code matches the style guide
• Unit tests are included in the PR
• For new widgets, an entry has been added to the .dojorc
• For new widgets, theme.variant() is added to the root domnode
• Any widget variant uses theme.compose like this
• WidgetProperties are exported

Description:

This pull request fixes an issue with TimePicker that prevented required instances from failing validation.

Resolves #1714

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

widget-test-docs – ./

🔍 Inspect: https://vercel.com/dojo/widget-test-docs/6eYDhPjwJBeJujeHg2xKwuwBLXar
✅ Preview: https://widget-test-do-git-fork-bitpshr-fix-time-picker-validat-2fb2f2.vercel.app

dojo.widgets – ./

🔍 Inspect: https://vercel.com/dojo/dojo.widgets/3mnjkR1Gi5wnCYQhCn4SxFj5ZaAh
✅ Preview: https://dojowidgets-git-fork-bitpshr-fix-time-picker-validate-dojo1.vercel.app

[codecov]

Codecov Report

Merging #1716 (1434215) into master (afc67b0) will increase coverage by 0.07%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1716      +/-   ##
==========================================
+ Coverage   90.17%   90.24%   +0.07%     
==========================================
  Files          94       94              
  Lines        5088     5116      +28     
  Branches     1384     1392       +8     
==========================================
+ Hits         4588     4617      +29     
+ Misses        247      244       -3     
- Partials      253      255       +2     

Impacted Files	Coverage Δ
src/time-picker/index.tsx	82.41% <66.66%> (+0.59%)	⬆️
src/date-input/date-utils.tsx	97.43% <0.00%> (-2.57%)	⬇️
src/date-input/index.tsx	91.37% <0.00%> (+0.30%)	⬆️
src/wizard/index.tsx	92.30% <0.00%> (+0.47%)	⬆️
src/select/index.tsx	88.79% <0.00%> (+1.83%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afc67b0...1434215. Read the comment docs.

[bitpshr]

This was the root of the issue: The underlying TextInput updates inputIsValid, but then local validation and validationMessages.length ended up overwriting this before triggering onValidate.

[tomdye]

@bitpshr should this be showing as visibly valid, ie. green, when set to required and with a value passed? It appears that it currently does not. Also, it looks like the dojo themed time picker now shows as valid / green as soon as it is rendered even though it's not supposed to.

[bitpshr]

@tomdye: The Material TimePicker does not show any valid border in its valid state (and neither do any other inputs, like TextInput), so I think we're good there. The Dojo TimePicker always showed a green border when initially rendered with a valid value, that is not related to these changes. Also note that any other input that wraps TextInput (like NumberInput) also render with a valid border initially when they have a valid value, see https://widget-test-docs-lnbpjip2w-dojo1.vercel.app/#widget/number-input/example/validation for an example (using the Dojo theme.)

I think we are good on both issues for this specific fix.

[agubler]

This works for me, I'd like to get @tomdye's approval and also a test to cover the changes?

[bitpshr]

@agubler test added that fails on master and passes here, made as simple as possible to show the issue. cc sir @tomdye when you have a chance to sign off.

[tomdye]

test looks good, but we've tried really hard to remove any need for these getRender / getTemplate functions via the addition of the assertion template so I'd rather we didn't add one here when you could simply to

const requiredTemplate = baseTemplate
			.setProperty('@input', 'required', true)

[tomdye]

Added a comment re. how setProperty should be used on the baseTemplate, once that's fixed please merge