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
540 stars 80 forks source link

Rewrite to component-specific props? #43

Closed cmeeren closed 5 years ago

cmeeren commented 5 years ago

I was looking through the MDN HTML attribute reference and noticed that most of the attributes are actually only valid for a very limited set of elements.

How would you feel about having element-specific types/modules, à la Feliz.MaterialUI? I'm not convinced it's the best solution for Feliz; I just want to put it out there.

Zaid-Ajaj commented 5 years ago

Interesting idea, it definitely makes sense for 3rd party libraries but I am not sure it fits in Feliz, I don't have a concrete example for why that is, maybe because I am unfamiliar with how the UI code will end up or because I will "have to think" too much about each I want to use. For now, let's keep the issue open, maybe people have something to say about it :)

MangelMaxime commented 5 years ago

In general, I think people know what they can or cannot use as an attribute for an HTML element.

Doing this kind of API asks a lot of effort, for example this is what is done for:

The difference between Feliz and my examples, is that in theory the API don't change often because of the "slow" evolution of the HTML language.

Also, this kind of API can lead to a more verbose code even if this probably mean going from

Html.div [
    prop.class "test"
]

to

Div.div [
    div.class "test"
]

// or

Html.div [
    div.class "test"
]
cmeeren commented 5 years ago

Yes, the last example is what I mean - I see no benefit to having each component in its own class.

The attributes that are truly global could go under prop to avoid excessive duplication.

Here's a bit more complete example (from the top of my head):

Html.div [
  prop.class "formWrapper"
  prop.children [
    Html.form [
      prop.children [
        Html.input [
          prop.class "emailInput"
          input.name "email"
          input.type.email
          input.autoComplete "email"
          input.autoFocus true
          input.value model.Email
          input.onChange (SetEmail >> dispatch)
        ]
        Html.input [
          prop.class "rememberMe"
          input.name "rememberMe"
          input.type.checkbox
          input.value model.RememberMe
          input.onChange (SetRememberMe >> dispatch)
        ]
        Html.button [
          button.disabled (model.Email = "")
          button.type.submit
        ]
      ]
    ]
  ]
]

The key benefit is increased ability to guide the user to correctness.

There are some drawbacks. IMHO Feliz should not make users think too much about where stuff is. With the above syntax, props are in either props or in <name of current component>, which I think is OK, but not 100% optimal since the user might not know which, and might have many "misses" in e.g. input before checking props. Also, it's all for nothing if users

know what they can or cannot use as an attribute for an HTML element

as @MangelMaxime said.

Again, I'm not sure that this is the best solution, but I'm not sure it isn't, either. At the moment I'm leaning slightly in favor of just keeping everything in props.

cmeeren commented 5 years ago

An inbetween solution could be to have all "enum" props defined separately to only surface allowed values for a specific element.

Related: #45.

Zaid-Ajaj commented 5 years ago

Thanks a lot @cmeeren and @MangelMaxime for the input. I agree with both of you really, especially this

There are some drawbacks. IMHO Feliz should not make users think too much about where stuff is - @cmeeren

and

know what they can or cannot use as an attribute for an HTML element - @MangelMaxime

I really want to minimize the number of places someone has to look for things, if users are used to go for prop as their first guess, then I would rather put stuff there even it doesn't make sense 100% it will makes things so much simpler and if there is a better solution we will definitely hear it.

I am wrapping up the issue for the stable release, let's close this one and come back to it when deemed necessary