search

mercurymedia / elm-datetime-picker

Single and duration datetime picker components written in Elm 0.19
https://package.elm-lang.org/packages/mercurymedia/elm-datetime-picker/latest/
MIT License
16 stars 6 forks source link

Picker UI improvements #28

Closed jmpressman closed 3 years ago

jmpressman commented 3 years ago

Changes

@rametta this should address the changes you requested from March. Apologies it took so long to get around to it. It's been a crazy year!

jmpressman commented 3 years ago

I don't know why we're introducing the SingleUtilities and DurationUtilities module. We could just keep them in the SingleDatePicker / DurationDatePicker as the functions are never used outside of this module.

I wanted to separate the code that defines the picker API & lifecycle (i.e. SingleDatePicker) from the code that is responsible for the business/data logic of the picker (i.e. SingleDatePickerUtilities). In my opinion the separation has made the code a lot easier to reason about as well as to determine what needs to be tested, two factors which should improve future maintainability.

Unfortunately, this PR also leads to some breaking changes that I'm actually fine with. However, we should at least create a CHANGELOG.md file and describe the changes theere. Maybe even a simple upgrade guide to make it easier to apply the changes to our projects.

Happy to get a Changelog/upgrade guide in place.

Tranquility commented 3 years ago

Yes, I think moving them into a separate module allows us test functions without actually exposing them to the user so this sounds like a good idea to me.

In general it is really hard for me to comment on the changes without actually using it. But I realized that the changelog is the only place we have documented some of the settings right now. When reading the documentation for the settings we could do a better job of explaining what the the fields are for (https://package.elm-lang.org/packages/mercurymedia/elm-datetime-picker/latest/SingleDatePicker).