gdotdesign / elm-ui

UI library for making web applications with Elm
https://elm-ui.netlify.com
BSD 2-Clause "Simplified" License
923 stars 39 forks source link

Cannot use Ui.DatePicker.subscriptions and Ui.DatePicker.subscribe when dealing with a list of date pickers. #53

Closed mrozbarry closed 7 years ago

mrozbarry commented 7 years ago

I've run into a problem subscribing to events and changes using Ui.DatePicker. I have a model that has a list of { Int, Ui.DatePicker.Model, String } where int is my own index/identifier, and string just converts the date picker selection into iso8601 for test purposes. When I don't use a list, everything seems to work fine. Once I introduce the list, even with a single element on it, I'm no longer able to see the select event and build my iso8601 string.

Another compound issue is that I'm not able to change months in the calendar picker.

I've built a minimal example here, and was wondering if this boils down to a problem with my implementation, or if there is a bug in elm-ui. Any help would be appreciated.

module Main exposing (..)

import Html exposing (div, span, strong, text)
import Date
import Time
import Debug

import Date.Extra.Format exposing (isoString)

import Ext.Date
import Ui.Container
import Ui.App
import Ui.DatePicker
import Ui

type alias Picker =
  { id : Int
  , datePicker : Ui.DatePicker.Model
  , iso8601 : String
  }

type alias Model =
  { app : Ui.App.Model
  , pickers : List Picker
  }

type Msg
  = UiAppMessage Ui.App.Msg
  | UiDatePickerMessage Picker Ui.DatePicker.Msg
  | SetIso8601 Picker Time.Time

init : Model
init =
  let
    datePicker : Ui.DatePicker.Model
    datePicker =
      Ui.DatePicker.init (Ext.Date.now ())

    picker : Int -> Picker
    picker id =
      { id = id
      , datePicker = datePicker
      , iso8601 = "Waiting for a date selection"
      }

    pickers : Int -> List Picker
    pickers count =
      count
        |> List.range 1
        |> List.map picker

  in
    { app = Ui.App.init
    , pickers = pickers 1
    }

update : Msg -> Model -> ( Model, Cmd Msg )
update msg model =
  case msg of
    UiAppMessage subMsg ->
      let
        ( app, effect ) = Ui.App.update subMsg model.app

      in
        ( { model | app = app }, Cmd.map UiAppMessage effect )

    UiDatePickerMessage picker subMsg ->
      let
        ( datePicker, effect ) = Ui.DatePicker.update subMsg picker.datePicker

        newPicker : Picker
        newPicker = { picker | datePicker = datePicker }

        replacePicker : Picker -> Picker
        replacePicker onPicker =
          if onPicker.id == newPicker.id then
            newPicker
          else
            onPicker

        updatePickers : List Picker
        updatePickers =
          model.pickers
            |> List.map replacePicker

      in
        ( { model | pickers = updatePickers }
        , Cmd.map (UiDatePickerMessage newPicker) effect
        )

    SetIso8601 picker time ->
      let
        iso8601 : String
        iso8601 =
          time
            |> Date.fromTime
            |> isoString

        newPicker : Picker
        newPicker = { picker | iso8601 = iso8601 }

        replacePicker : Picker -> Picker
        replacePicker onPicker =
          if onPicker.id == newPicker.id then
            newPicker
          else
            onPicker

        updatePickers : List Picker
        updatePickers =
          model.pickers
            |> List.map replacePicker

      in
        ( { model | pickers = updatePickers }
        , Cmd.none
        )

view : Model -> Html.Html Msg
view model =
  let
    viewPicker : Picker -> Html.Html Msg
    viewPicker picker =
      div
        []
        [ div [] [ text picker.iso8601 ]
        -- Is the problem here?
        , Html.map (UiDatePickerMessage picker) (Ui.DatePicker.render "en_gb" picker.datePicker)
        ]

    viewPickers : List (Html.Html Msg)
    viewPickers =
      model.pickers
        |> List.map viewPicker

  in
    Ui.App.view
      UiAppMessage
      model.app
      [ Ui.Container.column [] viewPickers ]

subscriptions model =
  let
    subscriptionsToPickerDatePickers pickers =
      let
        -- Is the problem here?
        subscribe picker = Sub.map (UiDatePickerMessage picker) (Ui.DatePicker.subscriptions picker.datePicker)

      in
        -- When I switch between Sub.none and my Sub.batch, I get different results.
        -- With Sub.none, my `subscriptionsToPickerDatePickerChanges` seems to do the right thing and I am able to update my iso8601 fields in my model
        -- With my Sub.batch, this doesn't work
        -- In both cases, I cannot change months
        Sub.none
        -- pickers
        --   |> List.map subscribe
        --   |> Sub.batch

    subscriptionsToPickerDatePickerChanges pickers =
      let
        -- Is the problem here?
        subscribe picker = Ui.DatePicker.subscribe (SetIso8601 picker) picker.datePicker

      in
        pickers
          |> List.map subscribe
          |> Sub.batch

  in
    Sub.batch
      [ subscriptionsToPickerDatePickers model.pickers
      , subscriptionsToPickerDatePickerChanges model.pickers
      ]

main =
  Html.program
    { init = ( init, Cmd.none )
    , view = view
    , update = update
    , subscriptions = subscriptions
    }
gdotdesign commented 7 years ago

I updated your code to make it work: https://gist.github.com/gdotdesign/31cde4cd3fd12fbac89341415fc5101a/revisions#diff-ac0369f042f6f2f81847c72c5fa3145e

There were two logical errors in it:

Most components in Elm-UI have unique ids and to generate them it uses native code and it's used to publish events for a component, so by doing this:

datePicker : Ui.DatePicker.Model
datePicker =
  Ui.DatePicker.init (Ext.Date.now ())

you create one record, and if you use this many times it will "broadcast" to the same channel. This can be solved by giving the function a dummy parameter:

datePicker : () -> Ui.DatePicker.Model
datePicker () =
  Ui.DatePicker.init (Ext.Date.now ())

The second one is using records in messages. There was a race condition between the subscribe and the messages of a datepicker, and since the records from the message were used instead of the ones in the model the state of the application was instabil, to demonstrate this:

  1. a record has the value test in the message, and there is a subscription that uses that message
  2. change the record to something in the model in the update function
  3. the subscription fires with the value test, but since the record changed before this the model would be overridden with the old value.

If you need more explanation let me know, I'll try to explain them better.

mrozbarry commented 7 years ago

@gdotdesign should I be using a similar pattern for Ui.Chooser messages (I posted an example a day or two ago as a separate issue) - what I mean is, should I be doing something like:

replaceChooser : MyChooser -> (MyChooser, Cmd Msg)
replaceChooser chooser =
  if chooser.id == changedId
    let
      ( uiChooser, effect ) = Ui.Chooser.update subMsg chooser.uiChooser
    in
      ( { chooser | uiChooser = uiChooser }, Cmd.map (UiChooserMessage changedId) effect)
  else
    ( chooser, Cmd.none )

It would be great if you could add some examples of this in your documentation - I'm definitely not the best when it comes to elm, but if I got stuck on it, someone else is bound to eventually, too.

mrozbarry commented 7 years ago

I just integrated into my test app locally, and everything is looking great. Thanks, I spun my wheels on this for a few hours yesterday, so I'm very appreciative of your help and getting back to me quickly!

gdotdesign commented 7 years ago

It would be great if you could add some examples of this in your documentation

This is not really an Elm-UI issue more like a general language one, however it could fit in a page like "best practices".

I just integrated into my test app locally, and everything is looking great. Thanks, I spun my wheels on this for a few hours yesterday, so I'm very appreciative of your help and getting back to me quickly!

I'm glad it works :)