Skip to content
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

DatePicker does not update value when changing year or month #98

Closed
gregg-cbs opened this issue May 10, 2024 · 14 comments
Closed

DatePicker does not update value when changing year or month #98

gregg-cbs opened this issue May 10, 2024 · 14 comments

Comments

@gregg-cbs
Copy link

When changing the month or year the date picker does not emit a change. Changes are only emitted when choosing a day:
image

I have tried the browseWithoutSelecting: false property but it does nothing.

version: 2.10.1

@gregg-cbs
Copy link
Author

It seems if you bind: the value changes happen... but the on:select is not fired when changing year or month

@gregg-cbs
Copy link
Author

I have now noticed that bind:value emits the value when showing the Date Picker. Almost as if the event is being fired in onMount or on some reactive property.

So if I do this:

<script>
  let value_datepicker = new Date();
  let show = false;

  function fireChange(date) {
    console.log(date) // fires whenever datepicker is shown and when the value changes
  }

  $: fireChange(value_datepicker)
</script>

{#if show}
<DatePicker
   bind:value={value_datepicker}
/>
{/if}

@probablykasper
Copy link
Owner

I know it's a bit confusing, but it's intended behaviour because you may not have selected your final value when you change the month/year. #88 would be the solution to this, so I'll close this as a duplicate of that

@probablykasper probablykasper closed this as not planned Won't fix, can't repro, duplicate, stale May 11, 2024
@probablykasper
Copy link
Owner

btw feel free to let me know if you have suggestions for how to name/differentiate between "date selection select" (on:select) and "value change"

@gregg-cbs
Copy link
Author

gregg-cbs commented May 12, 2024

I would consider any change in the date picker an on:select . Why splice this behaviour and introduce complexity.

If no date is chosen, dont emit until date is chosen. If the date picker has a date, then any change inside of it should emit a change event.

Now you have on:select and on:focusout and potentially talking of introducing confusing behaviour by firing on:select when the picker is closed and then an input event too.

All this stuff to dance around the fact that you only want to fire on:select on date click - why?

@probablykasper
Copy link
Owner

Because if select fired on every change, it would be confusing that it doesn't match the closeOnSelection option. If closeOnSelection happened every time the value changed, it would be very frustrating to use.

Now you have on:select and on:focusout and potentially talking of introducing confusing behaviour by firing on:select when the picker is closed and then an input event too.

Not sure what you mean, it would just be the same as how <input> has oninput and onchange

@gregg-cbs
Copy link
Author

closeOnSelection should be called closeOnDaySelection - because who are you to say what defines a selection? I select a year, i select a month, i select a day but now on:select only fires when i select a day. its confusing.

onInput does not emit a date, it emits the input event which myself and every user of this component has to extract the year or month from by doing e.target.value and somehow add that to our date.

I really think you should make a breaking change, bump this up a version and clean up this naming convention and functionality because any other way you do this is just opening you up to more questions and issues later on by the community.

You can even do something like this:

closeOnSelection: {day: true, month: false, year: false}

But on:select should fire the same way an on:change would, whenever a change happens. In fact i would argue that on:select should not even exist because its an anti-pattern. it should be on:change - and that should fire when there is a date and a change has happened - be that change on time, day, month, year.

@probablykasper
Copy link
Owner

Yeah that makes some sense, and browseWithoutSelecting does conflict with the select event.

But on:select should fire the same way an on:change would, whenever a change happens. In fact i would argue that on:select should not even exist because its an anti-pattern. it should be on:change - and that should fire when there is a date and a change has happened - be that change on time, day, month, year.

No, onchange doesn't fire whenever a change happens. on:change works exactly like our on:select

@gregg-cbs
Copy link
Author

Im talking about the way on:change works with every other component and native html element in the world - it emits a change whenever something is changed.

You are making this difficult by having some prism in your mind that you need on:select and it needs to only fire when a day is selected and if a year is selected then that is an on:input or on:change and closeOnSection only works for day selection but no other selection.

Please tell me what is your solution for allowing us to know when year, month or time has changed? How do we get the date object/string for when these changes happen? Maybe i am misunderstanding your approach.

@probablykasper
Copy link
Owner

Please read up on how the change event works in native elements. You simply don't know how it works, and I'm not interested in discussing that further

The solution to get every change is to subscribe to the value store atm.

I'm open to accepting PRs for these:

  • Add input event that fires whenever the value changes (but not for programmatic changes). Add input event #88
  • Make sure select event fires whenever a change is explicitly committed. Fire select when date picker is defocused #87
  • Rename select to change (non-breaking)
  • I'm undecided about closeOnSelect and browseWithoutSelecting

@gregg-cbs
Copy link
Author

gregg-cbs commented May 14, 2024

@probablykasper
Copy link
Owner

It's just that change wouldn't be the name because native change events aren't intended to fire on every change (see MDN). They do for <input type="radio"> and <select>, but not for <input type="date"> or <input type="text">. See here:

Screen.Recording.2024-05-14.at.22.50.06.mp4

@gregg-cbs
Copy link
Author

I mean if you want to split hairs and not call it on:change because some native html elements dont on:change on every change... then okay.

But considering DX, on:change is a pretty clear event name and describes what it does -it lets you know of changes. It is also a widely used naming convention on custom components.

But on:change, on:select - it is not why I started this discussion I was more worried about the on:select behaviour and that it was not firing on year, month and time changes and that you were thinking of introducing a separate event for those changes.

I appreciate our discussion on this and thanks for entertaining me. To stop being a pain to you I will fork this package and do what I need to do. And a big thanks for making this package so people like me can enjoy it, i appreciate it!

@probablykasper
Copy link
Owner

probablykasper commented May 15, 2024

I understand what you mean, but yeah I want to have an equivalent to the web standard change event, and I know people rely on it behaving this way. Good luck with the fork!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants