Orasund / elm-ui-widgets

Collection of reusable views for elm-ui.
https://orasund.github.io/elm-ui-widgets/
BSD 3-Clause "New" or "Revised" License
85 stars 11 forks source link

Make widget types match functions #2

Closed Orasund closed 3 years ago

Orasund commented 4 years ago

Discussion started in #1.

Make Dialog and ExpansionPanel type aliases match the dialog and expansionPanel function signatures.

The Widgets Types for Dialog and ExpansionPanel are outdated and therefore wrong.

Orasund commented 4 years ago

There is a much bigger problem here: This should not have happened in the first place. I don't have the time right now to explain, I'll need to investigate further.

But the best way to deal with this issue is to have a hotfix for version 2 and to then think of a good solution for version 3.

I'll give feedback to your PR tomorrow (i'm assuming you want a hotfix for version 2)

Orasund commented 4 years ago

My remarks regarding the hotfix can be found in #1.

For the long-term solution, I would like to try to explain the problem a bit:

The repository has two different structures. The actual code for Dialog and ExpansionPanel is located in Internal.Dialog and Internal.ExpansionPanel. The documentation of the package will meanwhile say that they are living in Widget. This results in a very flat module structure and from practical use I feel like this is the way to go.

The downside is that I now have code duplication. For functions, I can just point towards the internal implementation. The same does not work for types:

type alias Dialog =
  Internal.Dialog

Here the documentation does not really tell you how to construct such a type. So instead I just duplicated the code and hoped that I would remember to always apply changes to both definitions at once. (Which I didn't)

shaydon commented 4 years ago

Hi, and thank you. Regarding a hotfix, I have no urgent need for it as the dialog function is completely usable without the Dialog type. I was just bringing the inconsistency to your attention.

Of course one way to ensure consistency would be to give the type signature of the function in terms of the 'public' type alias, so:

dialog :
    DialogStyle msg
    -> Dialog msg
    -> List (Attribute msg)
dialog =
    Dialog.dialog

The downside is person reading the documentation now has an extra step to look up exactly what parameters dialog needs - but as the definition of Dialog appears just above perhaps that's not so bad?

Orasund commented 4 years ago

I often lookup the documentation by hovering over the function. in Visual studio. I noticed that in this exact example it takes 2 clicks to get to the definition, whereas i the solution I currently use needs no click at all.

A better solution would be

dialog : 
  DialogStyle msg 
  ->  { title : Maybe String
      , text : String
      , accept : Maybe (TextButton msg)
      , dismiss : Maybe (TextButton msg)
      }
  -> List (Attribute msg)
dialog =
  let
    fun : DialogStyle msg -> Dialog msg -> List (Attribute msg)
    fun =
      Dialog.dialog
  in
  fun

This way we get the best of both worlds.