dojo / widgets

:rocket: Dojo - UI widgets.
https://widgets.dojo.io
Other
88 stars 66 forks source link

fix: correctly validate required TimePicker #1716

Closed bitpshr closed 3 years ago

bitpshr commented 3 years ago

Type: bug

The following has been addressed in the PR:

Description:

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

Resolves #1714

vercel[bot] commented 3 years ago

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[bot] commented 3 years ago

Codecov Report

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

Impacted file tree graph

@@            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%) :arrow_up:
src/date-input/date-utils.tsx 97.43% <0.00%> (-2.57%) :arrow_down:
src/date-input/index.tsx 91.37% <0.00%> (+0.30%) :arrow_up:
src/wizard/index.tsx 92.30% <0.00%> (+0.47%) :arrow_up:
src/select/index.tsx 88.79% <0.00%> (+1.83%) :arrow_up:

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.

tomdye commented 3 years ago

@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 commented 3 years ago

@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.

bitpshr commented 3 years ago

@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.