Closed cmeeren closed 5 years ago
If the .reactElement
would go away, then the syntax would be OK-ish on it's own but it would look different when used with other third-party bindings but I think users can at least live with it:
Html.div [
Mui.appBar()
.position.absolute
.children([
Mui.typography()
.variant.h6
.color.primary
.text("Foo")
])
]
But I still think that duplicating the base properties would the easiest to do. I don't think you need to duplicate every single property because a lot of the times you won't use them, just duplicate the ones that are most likely to be used: id
, key
, className
, style
, children
and falling back to prop
for the very very rare cases. What do you think?
Continuing discussion of inheritance in https://github.com/cmeeren/Feliz.MaterialUI/issues/20 since this issue is only about the alternative syntax, which is a consideration in its own right separate from my struggles with inheritance (sorry if that wasn't clear).
I think the fluent syntax has another drawback, for example if i want to create an extensible control, i can use yield! to append new props. Or I can create union cases for limited props and do reduce to get the base props and yield it at anywhere I want. But with fluent syntax i can only append props in the end.
let myButton props =
Html.button [
prop.Classes [ ... ]
prop.Style [....]
yield! props
]
@albertwoo I'm not enturely sure what you mean by
Or I can create union cases for limited props and do reduce to get the base props and yield it at anywhere I want.
But there's no problem adding support for arbitrary props using the fluent syntax:
let myButton props =
Html.button
.classes([...])
.style([...])
.custom(props)
where custom
can have overloads accepting anything we want, really:
string * obj
In any case, for extensible components with the fluent syntax, you probably wouldn't pass props like that. You'd do something like this:
let myButton () =
Html.button
.classes([...])
.style([...])
And then use it like this:
myButton()
.text("foo")
I kind of like the props list style. In my code I did something like:
type ISimpleFieldProp = interface end
[<RequireQualifiedAccess>]
type SimpleFieldProp =
| Label of string
| Errors of string list
| OuterClasses of string list
| LabelClasses of string list
| ErrorClasses of string list
| FieldView of ReactElement
| OuterAttrs of IHTMLProp list
interface ISimpleFieldProp
let simpleField (props: ISimpleFieldProp list) =
let props = props |> List.choose (function :? SimpleFieldProp as x -> Some x | _ -> None)
let label = props |> UnionProps.tryLast (function SimpleFieldProp.Label x -> Some x | _ -> None)
let errorClasses = props |> UnionProps.tryLast (function SimpleFieldProp.ErrorClasses x -> Some x | _ -> None) |> Option.defaultValue []
let outerClasses = props |> UnionProps.concat (function SimpleFieldProp.OuterClasses x -> Some x | _ -> None)
let labelClasses = props |> UnionProps.concat (function SimpleFieldProp.LabelClasses x -> Some x | _ -> None)
div </> [
yield! props |> UnionProps.concat (function SimpleFieldProp.OuterAttrs x -> Some x | _ -> None)
match outerClasses with
| [] -> Style [ Margin "5px"; Padding "5px" ]
| cs -> Classes cs
Children [
if label.IsSome then fieldLabel labelClasses label.Value
match props > UnionProps.tryLast (function SimpleFieldProp.FieldView x -> Some x | _ -> None) with
| Some x -> x
| None -> ()
yield!
props
|> UnionProps.concat (function SimpleFieldProp.Errors x -> Some x | _ -> None)
|> fieldError errorClasses
]
]
With UnionProps.concat
I can merge all the things like IHTMLProp, Class string, Style etc and feed to Fable.React. Then I can use it like:
let simpleField1 props =
simpleField [
SimpleFieldProp.OuterClasses [ ... ]
]
let simpleField2 props =
simpleField1 [
SimpleFieldProp.OuterClasses [ ... ]
]
and all the OuterClasses can merge together. Of course if merge together is not I want I can just use UnionProps.tryLast.
</>
: I created for make two [] [] to [] for the existing Fable.React element. https://github.com/albertwoo/Fun.LightForm/blob/master/src/Fun.LightForm.Fable/Fable.React.Extra.fsThank you for the detailed example. I must admit that it has me confused. For example, you don't seem to be using Feliz at all, but normal Fable.React. So I'm having a hard time understanding how it's relevant to this discussion.
Also, merging prop values for the same props from several "inherited"/composed classes seems to me like an unnecessarily messy way to do extensibility given that in React/JSX based syntax (AFAIK) a component can have only one occurrence of any given prop.
It seems like what you are trying to do, is to defining a custom component that supports a handful of optional, custom props, some of which are props and some of which are child elements.
In Feliz (as it is now, without any fluent syntax), just off the top of my head, I might solve it like this (partial implementation; hopefully the rest of the props should be clear):
open Feliz
type Html with
member simpleField(
?label: string,
?errors: string list,
?labelClasses: string list,
?outerClasses: string list
?props: IReactProperty list) =
// Unsure if this is correct, since I don't know what UnionProps.concat actually
// does, but I'm sure you get the gist
let lbClasses = labelClasses |> Option.defaultValue []
Html.div [
yield! props |> Option.defaultValue []
outerClasses |> Option.map prop.classes |> Option.defaultValue (prop.style [ ... ])
prop.children [
match label with Some lb -> fieldLabel lbClasses lb | None -> ()
...
]
]
This also allows you to easily make any of the props required, if you desire.
As regards the fluent syntax under discussion, it would work the same way.
The usage of the example above would be:
Html.simpleField(
label = "foo",
outerClasses = ["bar"]
)
Note also that creating custom React components (e.g. function components with Fable.React's FunctionComponent.Of
) can be a great way to do reusability.
Yes, I am not using Feliz (I tried but there is no SSR support yet ). But I just want to explain my thoughts about prop list style.
Yes for merging props like class and style together would be a little mess, but it just make things simpler for me to write my app.
Your above example is very like the Fabulous Xamarin default way (I also use that project 😀). Sometimes it is not easy to extend simpleField. For example if I want to create a simpleField2
to be reused then i have to repeat the input parameters like:
type Html with
member simpleField2(
?label: string,
?errors: string list,
?labelClasses: string list,
?outerClasses: string list
?props: IReactProperty list) =
simpleField(label, errors, labelClasses, [ "bar"; yield! outerClasses ], props)
Appreciate you guys works!!!
:)
Possible workaround:
type SimpleFieldProps = {
Label: string option
Errors: String list
// ...
} with
static member Create(?label: string, ?errors: string list, .. ) = { .. }
type Html with
member simpleField (props: SimpleFieldProps) = // ...
member simpleField2 (props: SimpleFieldProps) = // ...
Usage:
Html.simpleField (SimpleFieldProps.create(..))
:) Thanks for the advise. This way can help but if I have a lot of props like in Fabulous Xamarin, it would be too much work. That is also why I also use Fabulous.SimpleElements (Thanks @Zaid-Ajaj again :))
Regarding the .reactElement
: AFAIK it's not really a problem, because we can make sure it's only required to use it when interoping with existing code that requires reactElement
. Otherwise, properties that accept ReactElement
in the fluent API (e.g. children
) can also have overloads accepting PropsBase
, internally calling .reactElement
.
However, a drawback then is that since lists can only contain one type, and there's no automatic upcasting inside a list, then we need an explicit upcast (e.g. to PropsBase
) anyway if we have more than one element. Unless there's a good solution to that, we might just as well keep reactElement
.
A possible workaround to the above is to not have lists of child elements, but use [<ParamArray>]
of PropsBase
. Then I think the parameters will be automatically upcasted. But then you need commas between each item. (You also replace []
with ()
, but I don't really care about that.)
There also needs to be an API for conditional props. In the current Feliz (and Fable.React) DSL we can selectively yield
props, but with chained prop calls as proposed here, that's not possible. This is a matter of API design, but I'm not convinced it'll be pretty, e.g. for enum props where there are no parameters - then we can't add a bool
parameter, which might be a solution for methods.
We could have a wrapping .conditional(bool, 'builder -> 'ignored)
, e.g.
.prop1("foo")
.conditional(false, fun p -> p.prop2("bar"))
.prop3("baz")
But it's not the prettiest API I have seen. (Not too bad, but the current conditional yield
is nicer IMHO.)
If I was writing the DSL in C#, this is the syntax I would have wanted to write for sure but since we have F# and all of it's nice list syntax, I very much rather use the current one instead of this
Gotcha. The only thing I really like about this syntax is that it's the most discoverable I have seen so far. Otherwise it seems to have too many drawbacks compared to the current one.
How do you feel about a syntax like this? (I drafted this quickly; there may be lots of room for improvement - for example, there may be a way to eliminate the
.reactElement
at the end of each component when more careful thought is given to this.)Benefits: Supports inheritance, and puts a final end to any discoverability issues still left in Feliz: No more hunting for the correct prop type; you're just dotting through and seeing what's available. Also supports overloaded props and enum props just like Feliz; we have basically just replaced property lists with "fluent builders".
Drawbacks: I don't know which impacts this has for bundle size or performance (see quick and dirty implementation below). Also it's "unusual" as far as Fable goes (no prop lists). Not that the latter matters by itself. Update: See more challenges here: https://github.com/Zaid-Ajaj/Feliz/issues/54#issuecomment-541658081
I'm not saying we should necessarily do this; I just wanted to get this out of my head and post it for discussion.
For the record, below is the implementation of the above syntax. I have not made any attempts to inline or erase stuff. And there's a bit of type trickery which would likely cause OO fanatics to spin in their graves (what with subtypes inheriting base types parametrized by the subtype and all).