Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider changing classes from String to Html.Attribute #1

Open
surprisetalk opened this issue Mar 30, 2018 · 1 comment
Open

Consider changing classes from String to Html.Attribute #1

surprisetalk opened this issue Mar 30, 2018 · 1 comment

Comments

@surprisetalk
Copy link

Hello!

I thought I'd continue the conversation from the other issue.

Let's take a look at the library as Strings:

stringExample = div [ class box ] []

If we want to add another class, we have a few obvious options:

stringsExample1 = div [ class box, class hasTextCentered ] []
stringsExample2 = div [ class <| box ++ " " ++ hasTextCentered ] []
stringsExample3 = div [ classList [ box, hasTextCentered ] ] []

And if we want to vary the alignment, we're looking at something like this:

empty = ""

stringsExample4 alignment
  = div [ class box
        , case alignment of
            Left   -> class empty
            Center -> class hasTextCentered
            Right  -> class hasTextRight
        ]
  [
  ]

By moving to Html.Attribute, we get something like this:

attrExample = div [ box ] []

Already much cleaner!

If we want to add another class, we have one straightforward path:

attrsExample = div [ box, hasTextCentered ] []

And to vary the alignment, we get something like this:

empty = class ""

stringsExample4 alignment
  = div [ box
        , case alignment of
            Left   -> empty
            Center -> hasTextCentered
            Right  -> hasTextRight
        ]
  [
  ]

For more on multiple class attributes, check out this issue.

So here's the tradeoff: moving to Html.Attribute will make most code much cleaner, but may make compound classes more difficult. Consider the following:

centeredBox = Html.Attribute
centeredBox = classList [ box, hasTextCentered ]

stringExample = div [ style [], centeredBox ] []

There's no way to transform multiple Html.Attributes into a single one, so we're stuck with the following:

centeredBox = List Html.Attribute
centeredBox = [ box, hasTextCentered ]

attrExample = div ( style [] :: centeredBox ) []

For me, this tradeoff is totally worth it -- which is why I've opened this issue!

Are there any problems that I'm overlooking?

@ahstro
Copy link
Owner

ahstro commented Apr 3, 2018

Thank you for the issue! ☺️

The reason I'm quite adamant about keeping String-versions of the classes is that this is meant to be a pretty low-level library, not necessarily appealing to most end-users, but more to (well, myself... and) authors of libraries such as your elm-bulma. I do not propose to anticipate all the ways someone might want to use the plain Strings, and would therefore not like to remove them.

I can definitely see the benefits of doing it both ways though, and with dead-code elimination coming in 0.19 (as I understand it), I don't see why we have to commit to either solution; I simply need to expose a different module with Html.Attribute-versions of the classes. That way, people wanting to use the Strings for lower-level applications could do so, and people just wanting to add a class to their div are free to do so.

If you agree that that is a valid solution, what do you think would be the best module naming scheme? I'm leaning between Bulma.Classes + Bulma.Attributes and Bulma.Classes.LowLevel + Bulma.Classes. The first scheme would be better because it would not require a major version bump, and the Attributes-part of the module name would make it quite clear what is exposed; the second scheme would be better because it follows the same naming pattern as elm-lang/dom, and the LowLevel-part is likely to point novice users to the more user-friendly option. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants