Consider changing all Attribute returning functions to return [Attribute] instead.

[ocharles]

I'm proposing a fairly big change to lucid, which is that all Attribute-forming functions instead return [Attribute]. The reason for this is that it composes considerably better when we want to group attributes. I'm using the htmx library, and one thing I'd like to do is factor out a little set of attributes I commonly use to connect an element to a server-sent event stream:

tailSse_ :: Link -> [Attribute]
tailSse link =
  [ hxSse_ $ "connect:" <> link
  , hxSwap_ "beforeend"
  , xOn_ "htmx:sse-message.camel" "$el.scrollIntoView(false);"
  ]

However, actually using this is a bit annoying. I end up needing to sometimes use [], and sometimes (x : y : tailSse_ ..):

imageStream :: ImageStream a -> Html ()
imageStream ImageStream{ streamLink, imageLink, images } =
  div_ ( class_ "flex flex-wrap gap-2"
       : tailSse_ streamLink 
       ) do

    -- This will be hidden by CSS if any images are actually in this <div>.
    div_ [ class_ "hidden only:block" ] do
      notice "No images have been captured."

    -- Show all images available now.
    traverse_ (viewThumbnail . imageLink) images

To me lucid is about consistency, and this is no longer very consistent. I could always use []:

imageStream :: ImageStream a -> Html ()
imageStream ImageStream{ streamLink, imageLink, images } =
  div_ ([class_ "flex flex-wrap gap-2"] <> tailSse_ streamLink) do
    -- This will be hidden by CSS if any images are actually in this <div>.
    div_ [ class_ "hidden only:block" ] do
      notice "No images have been captured."

    -- Show all images available now.
    traverse_ (viewThumbnail . imageLink) images

which is OK, but noisy, and I've had to give up using list syntax. Instead, if class_ returned [Attribute], I could do:

tailSse_ :: Link -> Attributes
tailSse link = mconcat
  [ hxSse_ $ "connect:" <> link
  , hxSwap_ "beforeend"
  , xOn_ "htmx:sse-message.camel" "$el.scrollIntoView(false);"
  ]

imageStream :: ImageStream a -> Html ()
imageStream ImageStream{ streamLink, imageLink, images } =
  div_ [ class_ "flex flex-wrap gap-2", tailSse_ streamLink ] do
    -- This will be hidden by CSS if any images are actually in this <div>.
    div_ [ class_ "hidden only:block" ] do
      notice "No images have been captured."

    -- Show all images available now.
    traverse_ (viewThumbnail . imageLink) images

This essentially means div_ :: [[Attribute]], and it applies concat itself.

Another option may just be to redefine Attribute as

newtype Attribute = Attribute [(Text, Text)]

at which point div_ doesn't need to change its type.

Sorry this is a bit rambly, but I hope it makes sense! Curious to hear your thoughts.

[ocharles]

An added bonus of this also lets us write things like

[ if foo then class_ "foo bar" else mempty ]

(Even better if we had when :: Monoid a => Bool -> a -> a!)

[chrisdone]

Interesting. I like the surface level syntax of this:

  div_ [ class_ "flex flex-wrap gap-2", tailSse_ streamLink ] do
    -- This will be hidden by CSS if any images are actually in this <div>.
    div_ [ class_ "hidden only:block" ] do
      notice "No images have been captured."

It's definitely more consistent and supports the simple case and also the flexible case.

I'm not sure about the types, whether it should be [[Attribute]], [Attributes] or whatever. I like a list of list because it's simple.

[chrisdone]

Opened a related issue for 'lucid2' https://github.com/chrisdone/lucid/issues/129

[ocharles]

I'm glad you like the idea!

I'm not sure about the types, whether it should be [[Attribute]], [Attributes] or whatever. I like a list of list because it's simple.

I'm also not sure. I lean the other way. I'm not sure I'd want to see a "got [Attribute] but expected [[Attribute]]" error. OTOH, you're unlikely to be able to construct a [Attribute] so maybe it's not a real issue. Either way, it does seem to leak implementation details a bit. It's also an API change (though as Attribute exposes its constructor, that's inevitable).

[chrisdone]

You raise a good point, type errors are a little confusing with nested lists. [Attributes] with Attributes as a monoid works for me if you don't have any objections to the name.

I think the discussion is already in breaking change land, so I wouldn't worry about API change as much as the best design.

[ocharles]

[Attributes] with Attributes as a monoid...

Yep, this would be my first choice.