ISSUE#1: Handle null dates #7

mkelandis · 2022-02-13T14:54:57Z

Creating new PR targeting your upstream repo instead of my forked repo - review changes applied. Will close my original local PR

…ultDate

…ned when a day is picked

probablykasper

Thanks again!

I updated the CI to work on PRs, so you can see that there are TypeScript issues that need to be fixed (and formatting which just needs npm run format).

src/lib/DateInput.svelte

probablykasper · 2022-02-13T21:32:28Z

src/lib/DatePicker.svelte

-  let dayOfMonth = value.getDate()
-  $: dayOfMonth = value.getDate()
+  let dayOfMonth = tmpPickerDate.getDate();
+  $: dayOfMonth = tmpPickerDate.getDate();


I think we might want dayOfMonth to be null when value is null

I don't think it can ever be null currently because there is always a default date.

tmpPickerDate can't, but value can be null. I was imagining that when DateInput is null, DatePicker should be null and as a result not have any date highlighted.

Right now, DatePicker starts as null but the defaultDate's dayOfMonth is highlighted, so to a user it looks like that's already selected.

IMHO - until they click on a specific day the value should stay unassigned - otherwise as soon as they launch the picker the date will get assigned to the default without the user actually selecting a day as a side effect.

Updating dayOfMonth doesn't update the value, so that wouldn't happen. If dayOfMonth is null, we would make sure no date shows up as selected before the user actually selects a date

Right now, DatePicker starts as null but the defaultDate's dayOfMonth is highlighted, so to a user it looks like that's already selected.

^-- Agree that this is still an issue. What do you think about having the picker show the default date in a lighter highlighted style - similar to the placeholder? Then when they select it it changes style to what you have now or something that clearly indicates the date is selected?

I don't think anything should be highlighted by default, really

...so then the default date would effectively be a default month and year to start the picker with no day selected - i don't think it's being used for anything else at this point.

Not sure about the correct UX for this, but I searched for "w3c" and "date picker" and the first example shows the date highlighted in the picker before it's selected (and before it assigns the value):
https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/datepicker-dialog.html

I think it's pretty common to not have a date highlighted when no date is selected. Blueprint and Airbnb both do it.

If DatePicker uses a default value anyway, I don't think it would make sense to allow it to be null

Implemented on my nullable branch

src/lib/DatePicker.svelte

src/routes/docs.md

probablykasper

Breaking change notes:

DatePicker, DateInput: value is now Date | null
DatePicker, DateInput: value defaults to null
DateInput: value becomes null when the input field is cleared

probablykasper · 2022-02-14T17:44:28Z

.gitignore

@@ -2,3 +2,5 @@
 /.svelte-kit
 /package
 /coverage
+/.idea/


Suggested change

/.idea/

Done on my nullable branch

src/lib/DateInput.svelte

src/lib/DatePicker.svelte

src/lib/parse.ts

codecov-commenter · 2022-02-14T23:13:12Z

Codecov Report

Merging #7 (1164753) into master (bec526b) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master       #7      +/-   ##
==========================================
+ Coverage   98.03%   98.05%   +0.01%     
==========================================
  Files           4        4              
  Lines         153      154       +1     
  Branches       41       42       +1     
==========================================
+ Hits          150      151       +1     
  Misses          3        3

Impacted Files	Coverage Δ
src/lib/date-utils.ts	`100.00% <100.00%> (ø)`
src/lib/parse.ts	`100.00% <100.00%> (ø)`

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 bec526b...1164753. Read the comment docs.

src/lib/DatePicker.svelte

probablykasper · 2022-02-15T21:34:08Z

src/lib/DatePicker.svelte

+  let shownDate = value ?? defaultDate
+  $: shownDate = value ?? defaultDate


Suggested change

let shownDate = value ?? defaultDate

$: shownDate = value ?? defaultDate

/** The date shown in the popup, for when `value` is null */

let shownDate = value ?? defaultDate

$: if (value) shownDate = value

We shouldn't reset the shownDate to default when the value is set to null

Fixed this on my nullable branch

probablykasper · 2022-02-16T07:45:56Z

Found a couple bugs:

If a date is selected, and you delete all text at once, invalid stays as false. However, if you delete it one character at a time, invalid is true.
When the datepcker is focused and the value is null, using the arrow keys does not select a new value (you can focus the datepicker by clicking somewhere there is no button)

mkelandis · 2022-02-16T14:48:32Z

When the datepicker is focused and the value is null, using the arrow keys does not select a new value

^-- If we decide to hide the default day in the picker per the earlier suggestion this issue goes away (I think). Right now I don't have a strong opinion about it. For this PR I will see if we can get the arrow keys to select the value.

Will try to address both of these shortly. Also feel free to put up piggy back prs with your suggested changes and I can just merge them into mine.

probablykasper · 2022-02-16T22:10:23Z

Also feel free to put up piggy back prs with your suggested changes and I can just merge them into mine.

Pushed them all into here. We should be pretty much good to go.

If we decide to hide the default day in the picker per the earlier suggestion this issue goes away (I think)

So right now with my change to not highlight any date, using the arrow keys will select a date relative to shownDate (even when no date is highlighted). I think it's nice for them to select something so that you can still select dates via keyboard. Maybe it would be better if it selects from an edge (e.g LeftArrow selects top-right value), or relative to the current date, but I'm fine with how it is now either way.

mkelandis · 2022-02-17T14:46:03Z

Resolved conflicts and updated the PR - we should be good to go but feel free to poke at it more.

probablykasper · 2022-02-17T17:37:46Z

Thanks!! :D

mkelandis and others added 10 commits February 11, 2022 11:34

README driven development changes to separate the value from the defa…

317405a

…ultDate

ISSUE#1: Code review changes

322d6e1

ISSUE#1: abstract a temporary date used by the picker that gets assig…

4990b72

…ned when a day is picked

ISSUE#1: Remove debugging

4dfe06a

ISSUE#1: Remove console logging from date-utils

6730f4d

ISSUE#1: Update docs with additional default date parameter

e062eb9

ISSUE#1: Break configurable defaultDate into a separate PR from this one

0a8bc31

ISSUE#1: Readme updates to mention value assignment behavior

f140cbe

ISSUE#1: Revert placeholder text per review feedback

1c31615

Merge branch 'master' into pr/7

c0eb2d3

probablykasper requested changes Feb 13, 2022

View reviewed changes

mkelandis added 4 commits February 14, 2022 09:58

ISSUE#1: Formatting fixes

d165220

ISSUE#1: svelte-check fixes

358058c

ISSUE#1: code review suggestion

306aec0

ISSUE#1: Requested docs change

8291989

mkelandis requested a review from probablykasper February 14, 2022 15:39

probablykasper requested changes Feb 14, 2022

View reviewed changes

mkelandis added 4 commits February 14, 2022 15:50

ISSUE#1: Adjust the value if a date has been selected

cfbd8d5

ISSUE#1: Escape the pipe character in the docs md

960b290

ISSUE#1: Fix comment

18b984d

ISSUE#1: format

1164753

probablykasper added 2 commits February 15, 2022 01:03

Rework value updating

3f7e34e

Rename tmpPickerDate to shownDate

b44cce4

probablykasper reviewed Feb 15, 2022

View reviewed changes

src/lib/DatePicker.svelte Outdated Show resolved Hide resolved

mkelandis added 2 commits February 15, 2022 13:04

Merge remote-tracking branch 'upstream/nullable' into ISSUE#1

42d821a

ISSUE#1: Validate shown date against the min/max (instead of value)

941d04e

probablykasper reviewed Feb 15, 2022

View reviewed changes

mkelandis and others added 6 commits February 16, 2022 09:49

Merge remote-tracking branch 'upstream/master' into ISSUE#1

ccec9bd

ISSUE#1: Empty string is always valid

e5af842

ISSUE#1: arrow behavior

0f61001

Keep shownDate when value is set to null

9a50650

Remove ./idea/ from gitignore

9463a54

Don't highlight a date if none is selected

a22d435

Merge remote-tracking branch 'upstream/master' into ISSUE#1

616b7fa

probablykasper merged commit 876723c into probablykasper:master Feb 17, 2022

probablykasper mentioned this pull request Feb 17, 2022

Handle nullable date. #1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISSUE#1: Handle null dates #7

ISSUE#1: Handle null dates #7

mkelandis commented Feb 13, 2022

probablykasper left a comment

probablykasper Feb 13, 2022

mkelandis Feb 14, 2022

probablykasper Feb 14, 2022

mkelandis Feb 14, 2022

probablykasper Feb 14, 2022

mkelandis Feb 14, 2022

probablykasper Feb 14, 2022

mkelandis Feb 14, 2022 •

edited

probablykasper Feb 14, 2022

probablykasper Feb 16, 2022

probablykasper left a comment

probablykasper Feb 14, 2022

probablykasper Feb 16, 2022

codecov-commenter commented Feb 14, 2022

probablykasper Feb 15, 2022

probablykasper Feb 16, 2022

probablykasper commented Feb 16, 2022

mkelandis commented Feb 16, 2022

probablykasper commented Feb 16, 2022

mkelandis commented Feb 17, 2022

probablykasper commented Feb 17, 2022

		let shownDate = value ?? defaultDate
		$: shownDate = value ?? defaultDate

ISSUE#1: Handle null dates #7

ISSUE#1: Handle null dates #7

Conversation

mkelandis commented Feb 13, 2022

probablykasper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkelandis Feb 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

probablykasper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 14, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

probablykasper commented Feb 16, 2022

mkelandis commented Feb 16, 2022

probablykasper commented Feb 16, 2022

mkelandis commented Feb 17, 2022

probablykasper commented Feb 17, 2022

mkelandis Feb 14, 2022 •

edited