Closed cmeeren closed 5 years ago
Hello,
I can't find in the source where div
accept a single ReactElement
can you please point me to the source code?
What does the user code look like when doing so?
https://github.com/Zaid-Ajaj/Feliz/blob/ac2419082ad5122f8f318e91667fc4526abcbfa6/src/Html.fs#L96
I'd guess user code looks like:
Html.div (
Html.x [ ... ]
)
Ah yes, I missed this one thank you.
Personally, I would just implement Html.div (_ : ReactElement seq)
because it would make the syntax consistent:
// Dummy type definition for Fable REPL
type ReactElement = class end
type IProp = interface end
type Html =
static member inline div (value: ReactElement) : ReactElement = unbox ""
static member inline div (value: ReactElement seq) : ReactElement = unbox ""
static member inline div (value: IProp seq) : ReactElement= unbox ""
static member inline div (value: string) : ReactElement= unbox ""
let xx =
Html.div (
Html.div [
Html.div "Maxime"
Html.div "Mangel"
]
)
let yy =
Html.div [
Html.div [
Html.div "Maxime"
Html.div "Mangel"
]
]
When having passing a single child we have to use (...)
syntax so why not remove this one and always use [...]
syntax.
Fine by me. I'm not sure I want to do the same in Feliz.MaterialUI
because some component props explicitly require a single child component, but for a general div
it might be the best choice.
I have been thinking about this from the start but I have a couple of "problems" with this approach
First of all, using Html.div [ ]
like this will not compile because F# cannot resolve which static overload to use: ReactElement list
vs. IReactPropertly list
and I think users expect this to "just work"
Second is when refactoring : if you start writing your code while prototyping with the following snippet
Html.div [
Html.h1 "Fable"
Html.h2 "React"
]
then you realize "Oh, I actually need to add a class to the div", suddenly you have to move the whole structure back to the following syntax
Html.div [
prop.className "shiny"
prop.children [
Html.h1 "Fable"
Html.h2 "React"
]
]
I think Html.div [...]
syntax is nice for demos and examples but when it comes to using it in an actual application it is not really useful. Here I am assuming that the user almost always wants to add a class, key or other commonly used properties. Please let me know if you happen to use this pattern Html.div [ Html.div [ ... ] ]
a lot in your applications because I would to add the such features on a commonly-used patterns basis.
The Html.div (...)
is still nice for a couple of use-cases like Html.h1 (Html.strong "I am bold")
but the arguments I stated above could be used against these as well so I am not really sure whether I want to add these overloads to the mix (except for Html.fragment
because it is specifically useful when it takes a list of elements directly)
What do you guys think? @cmeeren @MangelMaxime
I agree that fragment
should be used instead of div
when you only want a list of children without props, and fragment
already seems to have this overload:
https://github.com/Zaid-Ajaj/Feliz/blob/dc905aa2dd0003bcc0ac46ff6440f318a43327ce/src/Html.fs#L11
So I'm happy. :)
First of all, using Html.div [ ] like this will not compile because F# cannot resolve which static overload to use: ReactElement list vs. IReactPropertly list and I think users expect this to "just work"
According to the REPL Fable is able to determine the correct overload:
type ReactElement = class end
type IProp = interface end
type Props =
| Width of float
| Height of float
interface IProp
type Html =
static member inline div (value: ReactElement) : ReactElement = unbox ""
static member inline div (value: ReactElement seq) : ReactElement = unbox ""
static member inline div (value: #IProp seq) : ReactElement= unbox ""
static member inline div (value: string) : ReactElement= unbox ""
let xx =
Html.div (
Html.div [
Html.div "Maxime"
Html.div [
Width 2.
]
]
)
let yy =
Html.div [
Html.div [
Html.div "Maxime"
Html.div [
Width 2.
]
]
]
@Zaid-Ajaj Your point make sense, personnally I not a big fan of div (value: ReactElement)
neither div (value: ReactElement seq)
for the reasons you listed.
If I was to use Feliz (didn't had the time to test it for real yet), I think I would prefer to always use prop.children
to keep consistent the code everywhere but perhaps it's just me :)
@MangelMaxime I think @Zaid-Ajaj specifically meant an empty list (literal []
).
@Zaid-Ajaj For completeness: A trick you can use to help F#'s overload resolution (which I've used in several libraries) is to implement some members (often the more general/generic ones) as extension members (in an auto-opened module). For example, if you have an overload that takes 'a
and another overload that takes 'a option
, you'd implement the 'a
member as an extension member. In this case, either of them could be an extension member and F# would pick the non-extension member when using []
. It would "just work", as you say. But again, just mentioning this for completeness; it seems we all agree that fragment
should be used if you need a container without props.
@MangelMaxime I think @Zaid-Ajaj specifically meant an empty list (literal []).
According to REPL it compiles too.
let yy =
Html.div [
Html.div [
Html.div "Maxime"
Html.div [ ]
]
]
it seems we all agree that fragment should be used if you need a container without props.
Yes and no. fragment
can have one prop which is key
for optimisation but no other props.
According to REPL it compiles too.
Huh! Strange, I remember having problems with empty lists matching multiple overloads in other contexts. Truly, F# overload resolution worketh in mysterious ways.
According to REPL it compiles too.
Yeah really weird, if I use the following, the compilation fails
type overloads =
static member inline func(xs: string list) = "string list"
static member inline func(xs: int list) = "int list"
overloads.func [ ]
|> printfn "%s"
but if I change string list
into #seq<string>
then it works but the compiler will make a choice for me
type overloads =
static member inline func(xs: #seq<string>) = "string list"
static member inline func(xs: int list) = "int list"
overloads.func [ ]
|> printfn "%s" // int list
Weird stuff
Again, I will keep this issue open until someone has better ideas, for now it seems that it won't help to have seq<ReactElement>
overload because you almost always will use props and refactoring the code would be harder
Strange. In any case, I'm happy without such an overload, I just wanted to mention it. :)
The #seq<string>
force the compiler to check if the given type can be down-casted into seq<string>
, so I guess this new check is helping the compiler in this cases.
Actually while thinking on this issue, I think that my assumption that you almost always need props is not really the case, especially for third-party libraries that already "include" classes and props in the default component implementation. Take reactstrap/Forms for example you have the snippet:
<Form>
<FormGroup>
<Label for="exampleEmail">Email</Label>
<Input type="email" name="email" id="exampleEmail" placeholder="with a placeholder" />
</FormGroup>
</Form>
Here, Form
and FormGroup
both do not have any props and they don't need any, so this would look something like this with Feliz
Form.form [
FormGroup.formGroup [
Label.label [
label.htmlFor "exampleEmail"
label.text "Email"
]
Input.input [
input.email
input.name "email"
input.id "exampleEmail"
input.placeholder "with a placeholder"
]
]
]
So for completeness sake, I will the seq<ReactElement>
overloads and probably remove the single child overloads of only ReactElement
as input
I notice that
div
can take a singleReactElement
, but not multiple. Would it make sense to add such an overload? I personally don't have a problem writingprop.children
and indenting one level further, but since it already hasReactElement
, it might make sense to also haveseq<ReactElement>
.