Zaid-Ajaj / Feliz

A fresh retake of the React API in Fable and a collection of high-quality components to build React applications in F#, optimized for happiness
https://zaid-ajaj.github.io/Feliz/
MIT License
544 stars 82 forks source link

styles and sublists #5

Closed zanaptak closed 5 years ago

zanaptak commented 5 years ago

Thanks for this, I'm trying it out immediately!

As it happens I was also working on a new DSL almost just like this. Wasn't a fan of the traditional double-list parameter style even after many months of giving it a chance. Rather than compete I'll choose to be happy you saved me a bunch of work. 😃

Not sure how you feel about alternative suggestions/ideas since it's your library, but throwing these out as the paths I was going down, for you to consider or ignore as you please.

One:

For styles I was preferring to have the valid options "attached", and not the somewhat verbose method of having separate option types where you repeat the css property. Also unit alternatives attached instead of extra parenthesized args.

style.display.flex
style.justifyContent.center
style.margin.px 20
style.padding.rem 1.5

Doesn't cover all the overloads but gets the most important primitives which I think is 80% of the value of the feature.

Example implementation (just returning strings for example purposes):

module style =

  module Util =
    let cssDecl a b = a + ":" + b + ";"
    // SRTP to allow any numeric for units
    let inline rem< ^a when ^a : ( static member get_One : unit -> ^a ) > ( value : ^a ) = string value + "rem"
    let inline px< ^a when ^a : ( static member get_One : unit -> ^a ) > ( value : ^a ) = string value + "px"
    let inline percent< ^a when ^a : ( static member get_One : unit -> ^a ) > ( value : ^a ) = string value + "%"
    // and so on...

  module display =
    let _val s = Util.cssDecl "display" s // freeform for unrepresented combinations
    let none = Util.cssDecl "display" "none"
    let block = Util.cssDecl "display" "block"
    let flex = Util.cssDecl "display" "flex"
    let inline_ = Util.cssDecl "display" "inline"
    // and so on...

  module margin =
    let _val s = Util.cssDecl "margin" s // freeform for unrepresented combinations
    let auto = Util.cssDecl "margin" "auto"
    let zero = Util.cssDecl "margin" "0"
    let inline rem v = Util.rem v |> Util.cssDecl "margin"
    let inline px v = Util.px v |> Util.cssDecl "margin"
    let inline percent v = Util.percent v |> Util.cssDecl "margin"
    // and so on...

Two:

Maybe controversial, but the children being in a nested sublist isn't really that valuable from a coding ergonomics standpoint, beyond React doing it that way. It's an extra wasted indent that will compound on every nesting level.

It can be a mixed list using interfaces or DUs, the Html helpers can partition by attributes vs. elements and build the necessary React API objects without imposing that structure on the dev. (This also eliminates the need for the prop.content helper since you can just use Html.text directly.)

Alternative version of front page example:

let render state dispatch =
    Html.div [
        prop.id "main"
        prop.style [ style.padding 20 ]

        Html.button [
            prop.style [ style.marginRight 5 ]
            prop.onClick (fun _ -> dispatch Increment)
            Html.text "Increment"
        ]

        Html.button [
            prop.styleList [ style.marginLeft 5 ]
            prop.onClick (fun _ -> dispatch Decrement)
            Html.text "Decrement"
        ]

        Html.h1 state.Count
    ]

Three:

Any reason Html is capitalized while everything else is lower?

MangelMaxime commented 5 years ago

For styles I was preferring to have the valid options "attached", and not the somewhat verbose method of having separate option types where you repeat the css property. Also, unit alternatives attached instead of extra parenthesized args.

Seems interesting :thinking:

Maybe controversial, but the children being in a nested sublist isn't really that valuable from a coding ergonomics standpoint, beyond React doing it that way. It's an extra wasted indent that will compound on every nesting level.

It does not only react doing HTML is also doing it that way, but you do also have a list of children for a given element.

By using a prop.content property this allows us to do only one iteration over the list in order to create an object and not two which increase potentially the performance, and also reduce the bundle size increase to zero on this specific point.

It can be a mixed list using interfaces or DUs, the Html helpers can partition by attributes vs. elements and build the necessary React API objects without imposing that structure on the dev.

This helps structure the code so people don't mix children and properties like that:

prop.Id ""

Html.button [ ]

prop.style [ ]

Html.h1 [ ]

// etc

Let's keep things separate please :smile:

Zaid-Ajaj commented 5 years ago

Thanks for this, I'm trying it out immediately!

As it happens I was also working on a new DSL almost just like this. Wasn't a fan of the traditional double-list parameter style even after many months of giving it a chance. Rather than compete I'll choose to be happy you saved me a bunch of work. 😃

Thanks for trying this out, we are still testing the waters with how it will look like in a large app, keep us posted please :smile:

For styles I was preferring to have the valid options "attached", and not the somewhat verbose method of having separate option types where you repeat the css property. Also unit alternatives attached instead of extra parenthesized args.

I love this! Of couse it will not work for all properties because some of them like margin and border would want different units for different parameters so we can't replace the functions altogether but I would like to add the attached properties for distinct properties like display and textAlign etc. this would simplify a lot getting rid of specific types such as IDisplay and ITextAlignment etc. but also conditional styles are easier:

style.display (if true then display.none else display.block)

// vs

if true then style.display.none else style.display.block

This is the implementation I would like to follow:

[<Erase; RequireQualifiedAccess>]
module style =
    [<Erase; RequireQualifiedAccess>]
    type display =
        static member inline flex : IStyleAttribute = Interop.mkStyle "display" "flex"
        // etc.

but also keep things like style.margin 5 valid next to style.margin.px 5 and style.margin.vh 100

@zanaptak Can you please help out with these changes :pray:? the types and docs are already there but they need to be moved around.

As for the second proposition

Maybe controversial, but the children being in a nested sublist isn't really that valuable from a coding ergonomics standpoint, beyond React doing it that way. It's an extra wasted indent that will compound on every nesting level.

It can be a mixed list using interfaces or DUs, the Html helpers can partition by attributes vs. elements and build the necessary React API objects without imposing that structure on the dev. (This also eliminates the need for the prop.content helper since you can just use Html.text directly.)

Although I personally liked this in the beginning before implementing this library, I don't think it is a good idea now because it breaks compatibility: props and react elements would have to have the same type in order to put them in the list, this means that props would be some kind of elements or elements would have to some kind of props and they won't compose anymore with third-party libraries. Also this follows exactly the structure of React so I would like to keep it as is right now

Zaid-Ajaj commented 5 years ago

Implemented attached styles :smile: and will be available in v0.16! see README

zanaptak commented 5 years ago

It does not only react doing HTML is also doing it that way, but you do also have a list of children for a given element.

Not sure if I was clear enough, I'm referring specifically to putting child html elements under prop.children [] instead of directly under the parent Html.div [] element. This is arising directly from the chosen list representation of React props and is not like writing HTML (or even JSX I don't think, but not too familiar), e.g. we don't put child elements in a nested <children></children> or children="…" container in HTML, we put them right under the parent element.

By using a prop.content property this allows us to do only one iteration over the list in order to create an object and not two which increase potentially the performance, and also reduce the bundle size increase to zero on this specific point.

I'm guessing if necessary it should be possible to write a native JS function that could do it in one pass using mutable objects, I think it just needs to build a POJO and child array in-place unless there's more involved I'm overlooking. I don't really know much about bundle sizing so if that's an issue I don't have any ideas for that.

This helps structure the code so people don't mix children and properties like that:

This is a variation on problems that already exist by choosing a list DSL over a stricter abstraction, and is unreasonable to be singled out for special objection while the others are tolerated.

We can already split attributes in the new DSL.

Html.div [
    prop.id "id"
    prop.className "asdf"

    prop.children [

        Html.div [
            prop.content "child1"
        ]

        Html.div [
            prop.content "child2"
        ]

    ]

    prop.name "name"
    prop.key "key"
]

We can also duplicate and overwrite items -- which prop.children block wins?

Html.div [

    // Which children block will render?

    prop.children [
        Html.text "THIS ONE?"
    ]

    prop.children [
        Html.text "OR THIS ONE?"
    ]

    prop.id "id" ; prop.className "class"
    prop.name "name" ; prop.content "" ; prop.key "key"
    prop.style [
        style.display.flex
        style.backgroundColor.aquaMarine
    ]
]

(The answer is that the empty prop.content at the bottom clobbers them both.)

These are things we put up with in the name of convenience, that users already have to learn to avoid on their own without compiler help, the proposed alternative would fall into the same category.

@zanaptak Can you please help out with these changes 🙏? the types and docs are already there but they need to be moved around.

Sorry, I have to say no. Longer term I'm happy to contribute but for now you are in rapid-beta-release mode and I'm not prepared to be on the hook for any short-term promises.

Although I personally liked this in the beginning before implementing this library, I don't think it is a good idea now because it breaks compatibility: props and react elements would have to have the same type in order to put them in the list, this means that props would be some kind of elements or elements would have to some kind of props and they won't compose anymore with third-party libraries. Also this follows exactly the structure of React so I would like to keep it as is right now

Fair enough. Though you can cheat with compatibility by having props implement the empty ReactElement interface. If you were to change the grouping, and use a nested list for props instead of for child elements, you could isolate the cheat to a single appropriately documented helper.

Html.div [  // ReactElement seq -> ReactElement

    // Cheat: implements ReactElement, will "keyValueList" into props object internally
    props [
        prop.id "a"
        prop.style [
        // ...
        ]
    ]

    // Everything else become regular children
    // Any ReactElement (new DSL, old DSL, component, etc.)

    Html.div [
        // ...

    ]

    Html.div [
        // ...

    ]
]
MangelMaxime commented 5 years ago

Not sure if I was clear enough

You were pretty clear about the prop.children thing :)

We can also duplicate and overwrite items -- which prop.children block wins?

We can ask the same question for any property :) It will always be the last one who wins.

I still don't think mixing props and ReactElement is a good thing. However, your last proposition is interesting at first glance.

I don't see big cons about it because you still have the (visual) distinction between the props and children.

So depending on @Zaid-Ajaj opinion perhaps we can try to compare a big block of codes with the different styles to make a choice. It's already planned to do it between several other code format so one more or one less :)

zanaptak commented 5 years ago

After rereading this and looking at it a bit more I'm also thinking grouped props are better than the original mixed proposal.

Zaid-Ajaj commented 5 years ago

Sorry, I have to say no. Longer term I'm happy to contribute but for now you are in rapid-beta-release mode and I'm not prepared to be on the hook for any short-term promises.

@zanaptak That's fine, the feature got implemented already :)

Fair enough. Though you can cheat with compatibility by having props implement the empty ReactElement interface. If you were to change the grouping, and use a nested list for props instead of for child elements, you could isolate the cheat to a single appropriately documented helper.

I know it is easily possible, but it will require not only cheating the type-system but also the helper that needs to distinguish the props from the elements apart and that means I would to include metadata of each element plus metadata of the prop keyword to be able to distinguish them in runtime: this is not a goal I want to pursue in a base library where it should the least possible amount of helper and magic because code from here would be used over and over and it would start to hurt performance and bundle sizes, so it's a no-go

I will things as they are for now