fable-compiler / fable-arch

Framework for building applications based on the elm architecture.
https://fable-compiler.github.io/fable-arch/
Apache License 2.0
60 stars 14 forks source link

Can't combine classList and Class #37

Closed MangelMaxime closed 8 years ago

MangelMaxime commented 8 years ago

If we have the following code:

ul
  [ Class "dropdown-menu"
    attribute "role" "menu"
    classList [ "is-shown", true ]
  ]
  [ dropdownItemMenu { Text = "Se déconnecter"; Action = SignOut }
  ]

Only the classList attribute will be used.

Output html:

<ul role="menu" class="is-shown"><li>Se déconnecter</li></ul>

Expected result would be:

<ul role="menu" class="dropdown-menu is-shown"><li>Se déconnecter</li></ul>

Should we a make the attribute complementary and not exclusive ?

I think it's a constraint that we have to work with but we should probably document it.

Because if we make them complementary with would not be able to overwrite attribute if we want to. etc.

mastoj commented 8 years ago

I actually think that is an implementation detail of virtualdom. I'm pretty sure I pass both properties and attributes to virtualdom.

Class and classList are helpers you created, right?

MangelMaxime commented 8 years ago

Yes I created both (Source)

    /// Class attribute helper
    let inline Class value = attribute "class" value

    /// Helper to build space separated class
    let inline classList (list: (string*bool) seq) =
        list
            |> Seq.filter (fun (c,cond) -> cond)
            |> Seq.map (fun (c, cond) -> c)
            |> String.concat " "
            |> Class

And there both write to the same attribute value so it's "logic" to have mutual exclusion. I am just thinking can we have a way to make them complementary from our level. Because like you said it's certainly an implementation detail of virtualdom.

mastoj commented 8 years ago

we could maybe add a group by here: https://github.com/fable-compiler/fable-arch/blob/master/src/Fable.Arch/Fable.Arch.Virtualdom.fs#L26-L27 and concat the values separated by space.

Will that break other scenarios?

mastoj commented 8 years ago

I'm thinking of adding something like this:

let attrs = ["a","1"; "a", "2"; "b", "1"]

let a = 
    attrs
    |> List.groupBy fst
    |> List.map (fun (k, list) -> k, System.String.Join(" ", list |> List.map snd))
MangelMaxime commented 8 years ago

If we make the group specific for class attribute we shoudn't be breaking something else I think.

After, perhaps we could simply have a special helpers of this case.

let classBaseList b (list: (string*bool) seq) =
       list
            |> Seq.filter (fun (c,cond) -> cond)
            |> Seq.map (fun (c, cond) -> c)
            |> String.concat (b + " ")
            |> Class

After, having to much specialized helpers can make the code hard to read..

My worries by having the group is: Is there a case where we want to be able to overwrite a value ? Perhaps, we never need to do that.

MangelMaxime commented 8 years ago

My worries are wrong I think. If we don't want a class or something it will be up to the user to handle it. It's not the job, of Fable.Arch.

So yes, I think the solution adding a groupby is a good idea.

mastoj commented 8 years ago

That's my worries too. A helper is probably better. Or let the user create their own ;)

MangelMaxime commented 8 years ago

Ok. Let's add the helper.

And if we encounter this case another time it's will always be the time to make and update.

And I am working on a Library that will generate CSS for you with support of F# intellisense etc. So if the user want a more powerful manipulation over CSS (if block, loops, list support, etc.) there will have this library.

MangelMaxime commented 8 years ago

The PR #38 closed this issue. Shoud we open a new one to track the tasks of adding or we don't add this evolution ? @mastoj

I'm thinking of adding something like this:

let attrs = ["a","1"; "a", "2"; "b", "1"]

let a = 
    attrs
    |> List.groupBy fst
    |> List.map (fun (k, list) -> k, System.String.Join(" ", list |> List.map snd))
mastoj commented 8 years ago

I'm not sure we should add the group by stuff. Imagine a user creating another custom attribute, or whatnot, that concatenates with , instead of space. I might lean towards letting the user handle that by them self.

MangelMaxime commented 8 years ago

Seems fine to me. The helper fixed the problem for the current issue.