fable-compiler / fable-react

Fable bindings and helpers for React and React Native
MIT License
275 stars 66 forks source link

Why HTMLProp.Height attribute is commented ? #33

Closed MangelMaxime closed 7 years ago

MangelMaxime commented 7 years ago

Why we should not use Height attribute in HTML5 ? Indicated at this lines

Also, it's not the only properties conflicting with CSSProp, shouldn't we threat all the same way ?

alfonsogarciacaro commented 7 years ago

This was a PR some time ago but you're right, I've also wondered the same a couple of times. Probably it's the time to fix it. I can think of the following options:

MangelMaxime commented 7 years ago

I would simply uncomment it. The compiler will be there to help people detect the change and fix it.

Also, we do this change it can also be the right time to add data- case to avoid people doing !!("data-custom", "value")

alfonsogarciacaro commented 7 years ago

Yeah, you're right, but failures because of shadowed members are very tricky because people don't expect them. I always take a while until I figure them out and there are a couple of issues reported because opening Fable.Import.Browser shadows the FSharp.Core.Option module.

About data- maybe you could do something like this:

let inline Data(key: string, value: obj): IHTMLProp = !!("data-" + key, value)
MangelMaxime commented 7 years ago

Ah yes didn't think about adding this function to my project.

And I understand for the shadowed members, it cool also be a good time to think about improving the user Experience for SVG. Example of code:

let root model dispatch =

    svg [ Style [ CSSProp.Width "100px"
                  Height "100px" ] :> IProp
          ViewBox "0 0 42 42" :> IProp
          ClassName "marker" :> IProp ]
        [ circle [ Cx !^21.
                   Cy !^21.
                   R !^RADIUS
                   Fill "#fff" ]
            [ ]
          circle [ Cx !^21.
                   Cy !^21.
                   R !^RADIUS
                   Fill "transparent"
                   Stroke "#d2d3d4"
                   StrokeWidth !^3. ]
            [ ]
          image [ X !^"50%" :> IProp
                  Y !^"50%" :> IProp
                  Width !^30. :> IProp
                  !!("height", "30")
                  XlinkHref "assets/iconCrusher.png" :> IProp
                  Style [ TransformOrigin "50% 50%" ] :> IProp ]
            [ ] ]

As you can see there is a lot of !^ and also casting :> IProp. Usage of !^ is fine to me because it's the type system helping somehow. However casting every line because we used a props declared into the HTMLAtttr attribute is really annoying and noisy.

alfonsogarciacaro commented 7 years ago

Oh, I don't do much SVG myself so I didn't notice. Weird, I thought using a flexible type and making IHTMLProp inherit IProp would be fine. I need to check how this can be solved.

Can you please open a PR with the other proposed suggestions and we can work from there? Thanks!

MangelMaxime commented 7 years ago

Sure, will do.

In fact, I discover the "casting problem" with Elmish.Bulma but I found it less impactant as it's a library and only me know about the cast. Users don't see it.

MangelMaxime commented 7 years ago

I am closing as it's now fixed in 1.2.0