mdgriffith / design-discussion-elm-ui-2

A repo for discussing changes to Elm UI 2
17 stars 0 forks source link

Make `Element.Attribute msg`s more composable #22

Open rjdellecese opened 3 years ago

rjdellecese commented 3 years ago

The problem

Let's say that I have the following code:

dropShadow : Element.Attribute msg
dropShadow =
    Element.Border.shadow
        { offset = ( 0, 10 )
        , size = 0
        , blur = 20
        , color = black 0.15
        }

black : Float -> Color
black alpha =
    E.rgba255 0 0 0 alpha

And I use it in some places, e.g.

dropdown : Element msg
dropdown =
    Element.el
        [ Element.Font.size 14
        , dropShadow
        ]
        ...

However, I then decide that I want make my dropShadow to feel like it has a little more depth to it, by adding another Element.Border.shadow. Now its type signature must change.

dropShadow : List (Element.Attribute msg)
dropShadow =
    [ Element.Border.shadow
        { offset = ( 0, 10 )
        , size = 0
        , blur = 20
        , color = black 0.15
        }
    , Element.Border.shadow
        { offset = ( 0, 3 )
        , size = 0
        , blur = 6
        , color = black 0.25
        }
    ]

Which means that the places where it was invoked must change, too. But how? We've got a few different options, which is already a bad sign.

Option 1

dropdown : Element msg
dropdown =
    Element.el
        ( Element.Font.size 14
            :: dropShadow
        )
        ...

Option 2

dropdown : Element msg
dropdown =
    Element.el
        ([ Element.Font.size 14 ]
            ++ dropShadow
        )
        ...

Now let's say that I want to add another new style to this dropdown element, which looks like this:

square : List (Element.Attribute msg)
square =
    [ Element.width (Element.px 100)
    , Element.height (Element.px 100)
    ]

How should I accomplish this?

In the case where I previously went with Option 1, I might do this:

dropdown : Element msg
dropdown =
    Element.el
        ( Element.Font.size 14
            :: dropShadow
            ++ square
        )
        ...

And in the case where I had gone with Option 2, I might do this:

dropdown : Element msg
dropdown =
    Element.el
        ([ Element.Font.size 14 ]
            ++ dropShadow
            ++ square
        )
        ...

Ok, that works. Now let's say that I actually decide that I want square to only dictate the width of an element (my metaphors are breaking down here, apologies!). I could make one of two changes to accomplish this. Either

square : Element.Attribute msg
square =
    Element.width (Element.px 100)

or

square : List (Element.Attribute msg)
square =
    [ Element.width (Element.px 100)
    ]

The first of these is probably the most obvious one, which most users would prefer to make, but would then require further tweaking to all of the places where square is called. The second doesn't change the type signature and would thus invoke less churn at the call sites, but then raises questions like, "why not make all of my composable Element.Attribute msg functions wrapped in a List?" And also, to those less in-the-know, "why is this thing wrapped in a List?" Which again brings us to this problem of there being multiple ways to accomplish the same thing—always something to avoid, and something which we are usually especially good at avoiding in Elm!

One final example of where problems around the composition of Element.Attribute msgs arise is in cases where the expression might not even contain an attribute at all. Imagine that my original square function accepts a Bool which determines whether or not the element involved ought to be a square. How should I implement this? I again have multiple options.

square : Bool -> List (Element.Attribute msg)
square shouldBeSquare =
    if shouldBeSquare then
        [ Element.width (Element.px 100)
        , Element.height (Element.px 100)
        ]
    else
        []

or

square : Bool -> Maybe (List (Element.Attribute msg))
square shouldBeSquare =
    if shouldBeSquare then
        Just 
            [ Element.width (Element.px 100)
            , Element.height (Element.px 100)
            ]
    else
        Nothing

Which is better? More correct? If this were an Element msg I might use Element.none, but I have no such option here.

Ok, but what's the problem?

The problem here is that I'd like to treat Element.Attribute msg as a monoid—which is mostly just a fancy way of saying that I'd like to be able to reason about composing them independently of Lists. In all of the examples given above, I'd like it if the type signatures of my helper functions never had to change, regardless of the number of attributes that I add or remove as I develop my code. I want to be able to think about something like dropShadow as an Element.Attribute msg, and not have to be bothered in the type signature with whether its implementation involves just one Element.Border.shadow, or many (or even, potentially, none).

The solution

Thankfully, solving this problem is fairly simple and straightforward, and has precedence in Elm already—all we need are two new functions. The precedence for Cmd msg takes the shape of Cmd.none and Cmd.batch—same for Sub msg (Sub.none and Sub.batch). The particular names and name-spacing aren't that important to me, they just need to have the following type signatures:

-- Element

attrNone : Element.Attribute msg

batchAttr : List (Element.Attribute msg) -> Element.Attribute msg

If functions like these were available in elm-ui, there would be obvious best answers to the questions of "how do I implement this change?" in all of the previous examples—they could all be re-written to return Element.Attribute msg, exclusively.

mdgriffith commented 3 years ago

Nice. Thank you for writing this up!

Stacking shadows makes sense. I think I initially avoided that because each shadow is "expnsive", but in retrospect I think I was wrong 😅

I believe I already added a shadows : List Shadow -> Attribute msg to elm-ui 2.0 If not, I will likely add it.

So, here are the main things I think of with batched attributes.

  1. One of the reasons CSS is a pain is that it has indirection baked into it.

In order to figure out whats going on you have to look at every rule(in the worst case) that applies to an element.

  1. Batched attributes don't really give you any direction on what attributes should be grouped together.

Deciding which properties to group together is actually a surprisingly hard problem!

Here's a question: Are there attributes that, when grouped, would lead to subtle, rough situations?

Well, let's say I've chosen to create two groups

    one = 
        [ Background.color blue
        , Font.size 28
        ]

    two = 
        [ Background.color red
        , width (px 500)
        ]

Sure, it's obvious when we have these two literal definitions in front of us, but less obvious in a real code base.

Comparing things to Cmd is kinda tough because Cmds can't interfere with each other. The situation we have in elm-ui would be analogous to having behavior where adding another Http.get prevents a previous Http.get from firing! Oof

However! I recognize there's a painpoint here!

One approach that I've been most interested in is in having constructors for specific situations where we get that desire for binding two properties together.

So, one example of this would be allowing a constructor to take multiple shadows.

A few others could be:

The nice part about these groupings is that they push you towards using your design system properly.

So! Are there other grouping helpers we could create? There probably aren't a ton.

rjdellecese commented 3 years ago

Fantastic points! I'm going to try to state as concisely as I can all of the issues at play here.

  1. There are some styles that cannot exist simultaneously on the same element. Font.color is one example. A font may have exactly one color, and no more. So applying Font.color multiple times to the same element is always a non-sensical operation.
  2. There are some styles that can exist simultaneously on the same element. Element.shadow is one example. An element may have an arbitrary number of shadows, so applying multiple shadows will always have an effect (unless of course you apply the exact same shadow multiple times).
  3. There are some styles that always or very commonly have some kind of visual and/or semantic impact on other styles. For example, the various styles that affect font (weight, size, family), or font color and background color. Element border styles may be orthogonal to font styles, but may not necessarily be orthogonal to shadow styles.
  4. Styles applied to parent elements are currently inherited by their children. So when you have two styles that have a visual/semantic impact on each other, and one (styleOne) is applied to the parent element, the number of "correct" values of the other (styleTwo) in the child element are constrained by the value of the first (styleOne). But, it can be difficult sometimes to trace what the value of styleOne is, because it's applied out of scope of the application of styleTwo.

These raise a few questions/bring a few ideas to mind:

  1. Where clear groupings of interdependent UI styles can be identified, it would definitely be beneficial to at least encourage, if not require, users to specify them all together (as in your Font.with, Element.palette or shadows batching examples).
  2. Would elm-ui be easier to use if styles were not inherited by default?
  3. Given that [Font.color red, Font.color blue] is non-sensical, is there some way this kind of specification could be made invalid, or unlikely to occur?
  4. In light of all of the above, I still suspect that Element.Attributes.none and Element.Attributes.batch functions would still be useful under certain circumstances, although I agree that there are some other overlapping problems that they might be used to solve which could be better addressed by other means.
mdgriffith commented 3 years ago

Nicely said!

4./2. Only font attributes are inherited I believe, which is kinda necessary or you'd have to set font size/color on every node. 🤔 mostly the sort of indirection that I find confusing is when you have multiple lists of styles and concat them for one element.

  1. Yeah, it's tough or maybe impossible to enforce. Maybe if Element was el : AttributeRecord -> List (Element msg), but you could still update the record. And it would be a more awkward API.

The standard behavior is to always have a property overwrite a previously defined property. For shadow type stuff, maybe this means it makes sense to remove Border.shadow and only have a shadow constructor that takes a List Shadow. This would remove the ambiguity of stacking as there would be one obvious way to define multiple shadows.

  1. If we can find those other situations, would love to talk about them. Attr.none may be easier than Attr.batch, maybe you have an event handler that you only sometimes want to attach.
rjdellecese commented 2 years ago
  1. Yeah, it's tough or maybe impossible to enforce. Maybe if Element was el : AttributeRecord -> List (Element msg), but you could still update the record. And it would be a more awkward API.

Yeah, good points.

The standard behavior is to always have a property overwrite a previously defined property. For shadow type stuff, maybe this means it makes sense to remove Border.shadow and only have a shadow constructor that takes a List Shadow. This would remove the ambiguity of stacking as there would be one obvious way to define multiple shadows.

That is definitely an interesting idea! Seems worth exploring, IMO.

  1. If we can find those other situations, would love to talk about them. Attr.none may be easier than Attr.batch, maybe you have an event handler that you only sometimes want to attach.

I looked through my codebase again for better evidence in light of the good points you've made in this thread, and found what I think are the remaining use cases for Attr.none and Attr.batch functions:

  1. What you describe above; when I have an event handler that I want to sometimes attach. This occurs in my project, and also occurs with multiple event handlers vs. none (for example, I have a text editor which only has events attached to it when it is "editable", vs when it is "read-only").
  2. When I have a set of Html.Attribute msgs (via Element.htmlAttribute) that I want to apply all at once (or not at all), such as this one:

    noSelect : List (E.Attribute msg)
    noSelect =
        [ E.htmlAttribute (Html.Attributes.style "-moz-user-select" "none")
        , E.htmlAttribute (Html.Attributes.style "-ms-user-select" "none")
        , E.htmlAttribute (Html.Attributes.style "-webkit-user-select" "none")
        , E.htmlAttribute (Html.Attributes.style "user-select" "none")
        ]

    This would also apply for groups of Html.Attributes.propertys and Html.Attributes.attributes that you might have, as well (I have some of each of those, too).

rjdellecese commented 2 years ago

I recently read this blog post about the "phantom builder pattern", and realized that it could probably be employed to prevent non-sensical scenarios like the aforementioned "include two font colors in the same attributes list", e.g. [Font.color red, Font.color blue]. Roughly, this "phantom builder pattern" would allow you to track the number of times that a function has been applied to a term, at the type-level. I wonder what that would look like?

Here's a hypothetical API:

el : Attributes attrs -> Element msg

emptyAttrs : Attributes Attrs

{-| Likely there is a better name for this record than `Attrs`.
-}
type alias Attrs =
    { fontColor : InheritAttribute
    , shadows : UnsetAttribute
    -- and more
    }

{-| The attribute was set explicitly by the user on this node.
-}
type ExplicitAttribute =
    ExplicitAttribute

{-| The attribute is inherited from its parent.
-}
type InheritAttribute =
    InheritAttribute

{-| The attribute does not apply to the current node.
-}
type UnsetAttribute =
    UnsetAttribute

fontColor :
    FontColor
    -> Attributes { attrs | fontColor : InheritAttribute }
    -> Attributes { attrs | fontColor : ExplicitAttribute }

shadows :
    Shadows
    -> Attributes { attrs | shadows : UnsetAttribute }
    -> Attributes { attrs | shadows : ExplicitAttribute }

And that API in use:

el
    (emptyAttrs
        |> fontColor red
        |> shadows myShadows
    )
    (text "Hello world!")

One downside to this approach is that Attributes's type parameter needs to be fully-specified, or else the whole type signature must be omitted. In other words, this definition works, with the given type signature or without any type signature at all:

myAttrs : Attributes { fontColor : ExplicitAttribute, shadows : ExplicitAttribute }
myAttrs =
    emptyAttrs
        |> fontColor red
        |> shadows myShadows

But this one doesn't:

testAttrs : Attributes attrs
testAttrs =
    emptyAttrs
        |> fontColor red
        |> shadows myShadows

When the value of attrs is very large, as it would be in an real implementation in elm-ui, it would probably be preferred to just omit the type signature of a function with type Attributes attrs (but the monomorphic version—where attrs is a single, concrete type). But that's atypical in Elm code, and might be somewhat confusing to users.

Here's an Ellie with all of the above code stubbed out: https://ellie-app.com/ftHFPcQynt4a1

Anyways, I'm not sure whether an API like this would make sense for elm-ui, but I thought it would be neat to sketch out the idea, and in general thought it was neat that it's technically possible (and does offer some real benefit to the user)!