PanagiotisGeorgiadis / Elm-DatePicker

A DatePicker package created using Elm 0.19
https://package.elm-lang.org/packages/PanagiotisGeorgiadis/elm-datepicker/latest/
BSD 3-Clause "New" or "Revised" License
11 stars 2 forks source link

Add ability to pass the selectedDate to DatePicker.initialise #2

Closed icidasset closed 5 years ago

icidasset commented 5 years ago

Addresses #1

Does this work for you @PanagiotisGeorgiadis ? I could also make a initialiseWithDate and keep the original initialise if you think that's a better idea?

PanagiotisGeorgiadis commented 5 years ago

@icidasset Yeap it totally addresses the issue! One thing I would change is to pass a Maybe DateTime instead of a { selectedDate: Maybe DateTime } cause I think that since its going to be the only argument of DateTime type to be passed on our DatePicker, it should be clear enough.

Maybe on a future iteration we can change the multiple params into a record but I haven't given that a thought yet cause I think that they are declarative enough ( cause of their types ).

icidasset commented 5 years ago

Aight sounds good @PanagiotisGeorgiadis Updated it to your suggestion πŸ‘ Anything else you'd like me to do? (eg. bump version)

PanagiotisGeorgiadis commented 5 years ago

I feel pretty dumb @icidasset... I was just checking the API of the DateRangePicker module and it seems that in order for the user to set a DateRange I've introduced a setDateRange function which makes the implementation a bit more dynamic than just passing in a param on the initialise function. ( I've also introduced a resetVisualSelection function which resets it ).

Do you think that you would be able to implement it in this way cause I kind of see a benefit over the initialise function approach. I am so sorry for not seeing it earlier 😞

icidasset commented 5 years ago

@PanagiotisGeorgiadis No problem! ☺️

So, I'm not entirely sure I understand what you're saying. I assume you want me to implement a setDate function on the DatePicker module and leave the initialise function as is?

Also, the resetVisualSelection function is not applicable to the DatePicker module, right?

PanagiotisGeorgiadis commented 5 years ago

@icidasset Yeap exactly, let's implement a setSelectedDate function and leave the initialise as is. We could introduce a resetSelectedDate function ( which basically resets the selected date ), if we decide to create the setSelectedDate function like this:

setSelectedDate : DateTime -> Model -> Model
setSelectedDate = ....

resetSelectedDate : Model -> Model
resetSelectedDate = ....

This is more declarative than having the setSelectedDate accept a Maybe DateTime.

@icidasset Again I am so sorry that I didn't think of it earlier!

icidasset commented 5 years ago

@PanagiotisGeorgiadis Ayyeee, updated. I used different names than you suggested, which seem to fit better, but can change if you want to. Thanks.

PanagiotisGeorgiadis commented 5 years ago

@icidasset πŸŽ‰ Thanks for this, sorry to be obsessive, but I would really prefer that we stick with the originally proposed naming for two reasons: firstly that it leaves us consistent across the two modules that do similar things and secondly, so that the function name reflects the name of the record property in the model. Hope that's OK.

icidasset commented 5 years ago

@PanagiotisGeorgiadis No problem, updated πŸ‘

icidasset commented 5 years ago

Thank you! ☺️

PanagiotisGeorgiadis commented 5 years ago

The release is complete! Thanks a lot @icidasset! πŸ˜„