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

Support attributes in options #208

Closed
debois opened this issue Aug 25, 2016 · 13 comments
Closed

Support attributes in options #208

debois opened this issue Aug 25, 2016 · 13 comments

Comments

@debois
Copy link
Owner

debois commented Aug 25, 2016

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
Copy link
Owner Author

debois commented Aug 25, 2016

Issues and PRs related to this topic:

@debois
Copy link
Owner Author

debois commented Aug 25, 2016

Currently exposed attributes:

  • Button. onClick
  • Footer. onClick, href
  • Icon. onClick
  • Layout. onClick, href
  • Options. disabled (don't know if its used anyplace, though)
  • Slider. onChange (don't know if any of max, min, ... are actually attributes?)
  • Table. onClick.
  • Tabs.
  • Textfield. value, rows, cols, autofocus, maxlength,
    onInput, onBlur, onFocus, on
  • Toggles. onClick, group, value

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

vipentti commented Aug 26, 2016

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
Copy link
Owner Author

debois commented Aug 27, 2016

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
Copy link
Owner Author

debois commented Aug 27, 2016

I like @vipentti's experiment. We should

  • update Options.collect to either process attributes like Html.Attributes, or at least document the difference
  • go through our components and ensure that they "win" over user attributes and styling where they absolutely need to.

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

@vipentti
Copy link
Collaborator

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
Copy link
Owner Author

debois commented Aug 27, 2016

Uh! Exciting! Looking forward to seeing it!

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

No branches or pull requests

3 participants