debois / elm-mdl

Elm-port of the Material Design Lite CSS/JS library
Apache License 2.0
965 stars 133 forks source link

Support attributes in options #208

Closed debois closed 8 years ago

debois commented 8 years ago

Options currently support Html.Attribute.attribute only on Style m options. Other components laboriously wrap parts of Html.Attribute in Options to allow users customisation of various sorts. We need the latter to protect ourselves from users breaking elm-mdl by overriding important attributes, usually on "...".

Wrapping Html.Attribute is suspect because (a) it seems wasteful and inelegant the more attributes we add, and (b) we end up in situations where distinct components need the same attribute wrapped, and currently have no good solution for that. See #201.

I'd like to discuss whether the current approach of wrapping Html.Attribute values is the way forward, or if we should aim for something more general.

For starters, let's make a list of attributes currently supported and missing.

debois commented 8 years ago

Issues and PRs related to this topic:

debois commented 8 years ago

Currently exposed attributes:

Button, Menu, Toggles, both have "disabled", but I don't think we should think of those as attributes.

Helpers also expose a few, but let's disregard those for now; they're for internal use anyway.

(I might have missed a few.)

vipentti commented 8 years ago

One thing that I think needs to be considered that if we were to remove the restrictions from Options.attribute and allow generic Html.Attributes then even if we implemented #164 with #179 if users installed custom event handlers using Html.Attributes.on instead of Options.on then the Html.Attributes.on event handlers would overwrite Options.on or vice versa.

OvermindDL1 commented 8 years ago

That might get fixed considering the talk on the dev mailing list. They are looking at Html itself being able to take lists of attributes and be able to combine them properly. :-)

For now though, what about an 'arbitrary attribute' constructor for Options? Say it takes a function that takes a value(s) and returns an attribute, that would allow for being able to test and combine what you need. Could be used kind of like:

Options.attrWrap Textfield.Outer Html.Attribute.id "some-id"

Options.attrWrap Menu.InnerDiv Html.Events.onClick MyClickMsg

The first argument species the type, which can hold the config struct for the given item and thus do checking on it and return the final value that is either combined into the config or stored in it as a raw attribute if it is one that is not cared about.

Hmm, that is if function pointers are comparable in Elm... which I do not think so now that I think of it... Higher types could solve it too, but do not have those either... Blehg...

We really need to be able to see the Property type and introspect it, hate stupid hiding of needed things...

vipentti commented 8 years ago

On one hand I think we could open up Html.Attributes with a disclaimer and warning that if something breaks first you should check that one of the Html.Attributes is not the cause by overriding something important.

I think we could possibly mitigate this by ensuring that all Html.Attributes that have been provided by Options.attribute would be processed first (I believe latter attributes override former), this way our own internal attributes could override the attributes provided by the user if they for example try to override focus for Textfield.

OvermindDL1 commented 8 years ago

Hmm, that last suggestion might be the best work-around for now (with a note that custom settings may get overwritten). If Html.Attributes gets repeatable things added soon, which it looks like it might, that is best. Could also send a PR to Html core to make Property fully public so it can be introspected too.

vipentti commented 8 years ago

I believe problem with the Html.Attribute is that it is actually VirtualDom.Property and as such enabling introspection requires going Native and modifying the elm-lang/virtual-dom (which might or might not be acceptable)

OvermindDL1 commented 8 years ago

Considering I want the virtual-dom to be inspectable in its entirety... ;-)

Making a server-side elm renderer requires native code to inspect the virtual-dom right now, it is a pain, all because it is so opaque.

vipentti commented 8 years ago

While experimenting with this particular issue I came upon certain discrepancies between our Attribute versus result of using normal Html.Attribute.


-- This one outputs <div id="div-3"></div>
Html.div 
    [ Html.Attributes.id "div-1" 
    , Html.Attributes.id "div-2"
    , Html.Attributes.id "div-3"
    ]
    []

-- This one outputs <div id="div-4"></div>
Options.styled  Html.div 
    [ Options.attribute <| Html.Attributes.id "div-4"
    , Options.attribute <| Html.Attributes.id "div-5"
    , Options.attribute <| Html.Attributes.id "div-6"
    ]
    []

-- This one outputs <div id="div-7"></div>
Options.styled'  Html.div 
    [ Options.attribute <| Html.Attributes.id "div-7"
    , Options.attribute <| Html.Attributes.id "div-8"
    ]
    [ Html.Attributes.id "div-9" ]
    []

Now I'm not sure if this is actually an issue, as setting the same attribute several times should not really happen, but I thought it's worth mentioning.

debois commented 8 years ago

I like opening for arbitrary attributes with a big disclaimer. We need to reopen the Dispatch PR and support custom events first, though, because this is what people will do with it in the vast majority of cases.

I don't want people to have the disappointment of first thinking they can have the event handlers they need with attribute, then discover they can't do it without breaking elm-mdl.

debois commented 8 years ago

I like @vipentti's experiment. We should

(Lot of work in the last one. May not be worth it.)

vipentti commented 8 years ago

I think the second part is not actually as bad as it may seem. If we do this in v8 then we can perform some extra steps to ensure this. I already have somewhat working solution that does not require MAJOR version bump but needs further testing.

debois commented 8 years ago

Uh! Exciting! Looking forward to seeing it!