Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some components have {component}.children but others have to fall back to prop.children? #20

Closed
Zaid-Ajaj opened this issue Oct 2, 2019 · 29 comments

Comments

@Zaid-Ajaj
Copy link

I think it would be clean and consistent if children prop was added to all components so that you don't get this:

let render (state: State) (dispatch: Msg -> unit) =
  Mui.container [
    container.maxWidth.xl
    container.children [
      Mui.card [
        card.raised false 
        prop.children [ // <-- this line
          Mui.cardHeader [ ]
          Mui.cardContent [ ] 
        ]
      ]
    ]
  ]

but instead

let render (state: State) (dispatch: Msg -> unit) =
  Mui.container [
    container.maxWidth.xl
    container.children [
      Mui.card [
        card.raised false 
        card.children [ // much better
          Mui.cardHeader [ ]
          Mui.cardContent [ ] 
        ]
      ]
    ]
  ]
@cmeeren
Copy link
Collaborator

cmeeren commented Oct 3, 2019

Thanks, this should be fixed in the MUI docs: mui/material-ui#17674

Will re-generate when it's fixed.

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 3, 2019

Actually, there is no problem here. As indicated by mui/material-ui#17674 (comment), most MUI components forward "unknown" props to root elements. This is explicitly documented in the MUI API, and also in the Feliz.MaterialUI bindings:

https://github.com/cmeeren/Feliz.MaterialUI/blob/8ba1a2d212d4c396edd72c811324bda965bcc27a/src/Feliz.MaterialUI/Mui.fs#L30-L31

The semantically correct usage is therefore:

let render (state: State) (dispatch: Msg -> unit) =
  Mui.container [
    container.maxWidth.xl
    container.children [
      Mui.card [
        card.raised false 
        paper.children [
          Mui.cardHeader [ ]
          Mui.cardContent [ ] 
        ]
      ]
    ]
  ]

Feel free to close if you agree.

@Zaid-Ajaj
Copy link
Author

The semantically correct usage is therefore

Although it might be the "correct" usage, the propagation of children into the very top container type is (and should be) an implementation detail: it will confuse many users having to know the specifics of this.

My point is that we want to make looking for things (discoverability) as simple as possible without having to look in too many places, in this case when you are using Mui.card it would be great to add the things you will most likely use in the card type as well -> children

This also applies to things like button.onClick and button.text for Mui.button. Yes I know it is not entirely needed but it makes the lives of devs easier much much easier

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 3, 2019

I assume you don't want each component to separately define ALL the props of its root elements, and its root elements' root elements, and so on, all the way to native elements (essentially replicating the entire Feliz API for all components).

Under that assumption:

  • How can we determine which props to duplicate?
  • Since we're duplicating some props but not others, is the inconsistency something we can live with, or is that confusing for devs in itself? (E.g., "why does component X have prop Y when it's not listed in the MUI API?", or "why does component X have prop Y but not Z replicated from its root element?")

I like discoverability as much as the next dev (hence my working on Feliz.MaterialUI), but at the moment I'm inclined to think that when using Material-UI, it's simply required knowledge (and easily accessible) which components will forward props to which other components. It is certainly required when working with MUI in JS/TS.

That said, if we can improve upon that without too many caveats, I'm open for that. Furthermore, since things are auto-generated, it wouldn't be too hard to insert prop X from component Y into component Z, too, as long as we have pre-defined lists of which props to insert (or can infer that from the source docs somehow). It would be a bit of work to implement it, but once implemented, it would "just work". I would also need to make a solution for inserting native Feliz props.

@Zaid-Ajaj
Copy link
Author

I like discoverability as much as the next dev (hence my working on Feliz.MaterialUI), but at the moment I'm inclined to think that when using Material-UI, it's simply required knowledge (and easily accessible) which components will forward props to which other components. It is certainly required when working with MUI in JS/TS.

When working with Mui in JS, you write the code

<Card>
  <CardHeader>Title</CardHeader>
  <CardContent>Content</CardContent>
</Card>

You don't think about children being forwarded to paper - I certainly don't as I just learned this detail about it and if I had to learn and remember where what goes, then it already too high of an entry barrier for me

I assume you don't want each component to separately define ALL the props of its root elements, and its root elements' root elements, and so on, all the way to native elements (essentially replicating the entire Feliz API for all components).

That would actually be ideal, if it doesn't add to the bundle size and can be added to the generator, then I am all in for it. I think of Mui as a UI library that worked around inheritance by forwarding the props up to the parent but many UI libraries allow you to set properties of the specific element (i.e. text of button) because it inherits from a generic control which has text as well, since we can't use inheritence here (I have no idea how in any case) then duplicating the code might be a solution

How can we determine which props to duplicate

Can we hard code the inheritance hierarchy in a list and use that as a starting point? it should be doable, there aren't that many components after all

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 3, 2019

When working with Mui in JS, you write the code [...] You don't think about children being forwarded to paper

You're right, children is special.

That would actually be ideal, if it doesn't add to the bundle size and can be added to the generator, then I am all in for it.

Hm. Something to actually consider. 🤔 But replicating the entire Feliz UI? Then I'd have to re-generate every time Feliz changed, too. Added maintenance burden.

Side note: Since you're in favour of this, why not have the same kind of API for Feliz? There are, after all, several element-specific props for native components, too. (We've touched on this in a few issues in the Feliz repo.)

since we can't use inheritence here (I have no idea how in any case)

Check out the Elmish.WPF.Dynamic usage. Something like that might work, but I'm not sure if the API is necessarily better (it might be). It would also not be immediately compatible with vanilla Fable.React, though a simple helper function could solve that. Feel free to experiment!

Can we hard code the inheritance hierarchy in a list and use that as a starting point? it should be doable, there aren't that many components after all

That is certainly possible, though if feasible I'd rather just parse the hierarchy from the previously mentioned prop forwarding message.

@Zaid-Ajaj
Copy link
Author

Then I'd have to re-generate every time Feliz changed, too

Feliz props are static, they only change when the html specs change so that is not a big deal

Since you're in favour of this, why not have the same kind of API for Feliz?

Unlike elements in third-party libraries, Html elements are not specialized (with some exceptions ofc) so I am not sure this code would make sense

Html.div [
  div.style [ style.margin 5 ]
  div.children [
    Html.button [
      button.style [ style.padding 10 ]
      button.onClick (fun _ -> printfn "Hello")
      button.text "Click me"
    ]
  ]
]

Though if I look at it now, it is not bad at all. However, 3rd party libraries would have to consider extending existing props such button instead of creating a new type

Check out the Elmish.WPF.Dynamic usage.

I don't think I like that syntax, it is not as "declarative" as I would like to have it (though this is very subjective)

That is certainly possible, though if feasible I'd rather just parse the hierarchy from the previously mentioned prop forwarding message.

If it is possible that would be great, I see a couple of components say: "Any other props supplied will be provided to the root element (native element)" but it is not clear (from my understanding) which element is the native element of that specific component (such as bottomNavigation is that nav maybe?)

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 3, 2019

Feliz props are static, they only change when the html specs change so that is not a big deal

Assuming the Feliz API is stable, which is might not be. "Can you please add overload X", "you've forgotten prop Y", etc. 😅 Stable binding target API doesn't mean stable binding API 😉

I see a couple of components say: "Any other props supplied will be provided to the root element (native element)" but it is not clear (from my understanding) which element is the native element of that specific component

Yeah, if it says "native element" then I guess we could just copy the whole Feliz API.


Just had a brainwave: Would it work if instead of modules and static types, we have normal classes, and the button in button.style would just be a previously defined instance of that class (like with CE builders)? Because then we can do inheritance. Something like:

type paper () =
  member inline __.X ...
  member inline __.Y ...

let paper = paper ()

type card () =
  inherit paper ()
  member inline __.Z ...
  // inherits X and Y

let card = card ()

@Zaid-Ajaj
Copy link
Author

Would it work if instead of modules and static types, we have normal classes, and the button in button.style would just be a previously defined instance of that class

Looks like that would work 😉

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 3, 2019

That solves our problems then, doesn't it? If you do that in Feliz (e.g. a prop class) then I can easily derive from the Feliz props in Feliz.MaterialUI.

Heck, you could even easily have component-specific props using that method. As a first step, all component-specific prop classes can derive from a prop class containing everything. Then I can start deriving from element-specific prop classes (e.g. button), and you can clean up the API where relevant in your own time.

Edit: That won't work though, because MUI often allows you to change the underlying component (e.g. paper.component "nav"). So I have to inherit the whole Feliz prop API anyway, because I don't know at compile time which prop will be used.


Would the same [<Erase>] rules work for non-static classes? I.e., if all members are inline, we can erase it, and if there are non-inline methods, the class must not be erased?

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 3, 2019

Another complication for the inheritance stuff: According to mui/material-ui#17674 (comment), there are components that don't support all of their "root" props. For example, not all components support children even though they forward props to a root element that support children. So this can not be strictly modelled by inheritance. The question is whether that's a big enough problem to discard this idea.

Personally I don't like the idea of having speedDialAction.children when it's not supported.

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 3, 2019

It seems that MUI never forwards children, so that's a special case. (There may be more special cases for all I know.) mui/material-ui#17674 (comment)

@Zaid-Ajaj
Copy link
Author

There is another problem with inheritable prop classes: they cannot be extended with equivalent modules, this doesn't compile 😢

open Fable.Core 

[<Erase>]
type paperType() =
  member inline _.X = "one"
  member inline _.Y = "two"

let paper = paperType()

[<Erase>]
type cardType() =
  inherit paperType()
  member inline _.Z = "three"
  
let card = cardType ()

module card = 
  let hello = "whatever"

// card does not have hello definition
let xs = [card.X; card.Y; card.Z; card.hello]
printfn "%A" xs

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 3, 2019

No, but this does 😄

open Fable.Core 

[<Erase>]
type paperType() =
  member inline _.X = "one"
  member inline _.Y = "two"

let paper = paperType()

[<Erase>]
type cardHelloType() =
  member inline _.foo = "whatever"

[<Erase>]
type cardType() =
  inherit paperType()
  let cardHelloType = cardHelloType ()
  member inline _.Z = "three"
  member inline _.hello = cardHelloType
  
let card = cardType ()

// card does indeed have hello definition
let xs = [card.X; card.Y; card.Z; card.hello.foo]
printfn "%A" xs

@Zaid-Ajaj
Copy link
Author

Nice!

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 3, 2019

PS, I'd consider naming the type paperProps and cardProps etc.

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 3, 2019

Regarding my previous comment:

Heck, you could even easily have component-specific props using that method. As a first step, all component-specific prop classes can derive from a prop class containing everything. Then I can start deriving from element-specific prop classes (e.g. button), and you can clean up the API where relevant in your own time.

Edit: That won't work though, because MUI often allows you to change the underlying component (e.g. paper.component "nav"). So I have to inherit the whole Feliz prop API anyway, because I don't know at compile time which prop will be used.

In fact, we might yet get it working. We could parametrize the prop types by the root component's prop type, e.g. paperProps<navProps>. Then paperProps could have a property of type navProps that gives access to those props (getting dangerous though, because the prop name might be taken by "real" MUI props later). We could have instances paper (default) and paperWith nav to instantiate a paperProps<navProps> using the default nav props instance (perhaps we could also have it set paper.component "nav", though I'm not sure exactly how - perhaps SRTP and a suitable member on all native element prop types?).

Not ideal, but an idea nonetheless.


Regarding another previous comment of mine:

For example, not all components support children even though they forward props to a root element that support children. So this can not be strictly modelled by inheritance. The question is whether that's a big enough problem to discard this idea.

One workaround is to shadow inherited, unsupported children members by implementing a children member with an unusable type, such as a property returning a custom UnsupportedProp type that can't be used in the DSL. AFAIK a simple property like that will even shadow all method overloads.

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 7, 2019

Re. the Elmish.WPF.Dynamic syntax: A slightly better syntax in the context of Feliz may be the following:

Html.div (fun d ->
  d.style (fun s -> s.margin 5)
  d.children [
    Html.button (fun b ->
      b.style (fun s -> s.padding 10)
      b.onClick (fun _ -> printfn "Hello")
      b.text "Click me"
    )
  ]
)

In other words, the usage is exactly like the current syntax except instead of a prop list, there's an initializer function that gives you the parameter by which you access the props. So you never have to "hunt around" for the correct type/module where the available props are defined.

This may have more benefits in Feliz.MaterialUI than in Feliz. For example, all the classes props currently document that you should e.g.

Use classes.appBar to specify class names.

That instruction could be moot if one simply did

appBar.classes (fun c ->
  c.root "foo"
  c.positionFixed "bar"
)

// Existing syntax for comparison
appBar.classes [
  classes.appBar.root "foo"
  classes.appBar.positionFixed "bar"
]

In any case, I'm not about to go making this change unless you really like it and would use it in Feliz. It has benefits and drawbacks. I just thought I'd mention it.

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 7, 2019

Dang, I just thought of something. With the proposed instance-member syntax, it's not possible to have mixed enum and non-enum props. For example, I can't think of a way to achieve both of the following variants using instance members, because anchorOrigin needs to be both a property and a method, which isn't possible.

popover.anchorOrigin.topLeft
popover.anchorOrigin (10, 20)

The best I can come up with right now is to make the property a parameterless method, or use different names:

popover.anchorOrigin().topLeft
popover.anchorOrigin' (10, 20)

I'm not particularly fond of either, though if I had to choose between these I think I like the different name better than the property-as-method.

Still, I would very much like to enable inheritance, otherwise I doubt I'll fully implement the suggestions in this issue (I don't want to duplicate the entire Feliz API for each and every component).

Ideas welcome!

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 8, 2019

Wait a minute... Might we solve this without any inheritance, and instead use module and type aliases? For example:

image

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 9, 2019

Man, this issue is proving difficult.

I was hovering over the "commit" button with a complete solution, implemented in the generator and all, but figured I'd test it first just to be sure. And it turns out that I can't use this to inherit enum (module) props, because module aliases are only accessible within a file.

I tried doing the following (menuItem inherits listItem which inherits buttonBase):

[<AutoOpen>]
module InheritanceLevel2 =

  type menuItem = buttonBaseProps

module menuIteml = buttonBaseProps

[<AutoOpen>]
module InheritanceLevel1 =

  type menuItem = listItemProps
  type listItem = buttonBaseProps

module menuItem = listItemProps
module listItem = buttonBaseProps


[<AutoOpen>]
module InheritanceLevel0 =

  type menuItem = menuItemProps
  type listItem = listItemProps
  type buttonBase = buttonBaseProps

module menuItem = menuItemProps
module listItem = listItemProps
module buttonBase = buttonBaseProps

This works great for the prop types, but not the modules, whose aliases are not accessible outside this file.

So I'm back to start, really.

I could duplicate all props in the types/modules that inherit them, but that won't work for 3rd party inheritance (e.g. when inheriting base Feliz props) because I don't have those definitions (also, there's a LOT of them, and they would be present for all 120 MUI components, so compile times would likely be horrendous). I'd like, if possible, to have a solution where components can inherit props from 3rd party components (such as Feliz).

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 10, 2019

Continuing discussion from Zaid-Ajaj/Feliz#54 (comment):

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?

Not a bad idea. Though I still haven't warmed entirely to the idea of duplicating.

As I see it, there are two issues at work here:

Base Feliz props

If only a few properties are used frequently, and the rest of the properties are used rarely, then it could work well to duplicate only the frequently used base props for each MUI component.

However. I worry (perhaps prematurely?) that it may not be easy to choose the frequently used props (for example, in addition to your list, all button-like elements may have onClick, all input-like elements may have onChange, etc.) and that users might come with many requests to add their favorite props (because why not, the duplication is just based on a hunch after all), ultimately leading to lots of duplication anyway.

Furthermore, if prop overloads should be improved in Feliz (perhaps not that likely?), those changes should be done in Feliz.MaterialUI as well (otherwise users will have to use prop. if wanting the new overloads, making it inconsistent anyway).

Inheritance within MUI

Here I have full control over prop definitions and can easily duplicate (and even add "Inherited from X" to the docs for each property). So this isn't as much of an issue.

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 10, 2019

I have a slight twist on the latest idea (two comments up) I think may actually work.

The only trouble I had were with enum prop modules. Since we can't alias the modules, we can alias the prop types inside the modules instead. Seems like that'll have to work. I'll get back with details once I have tried.

If it works, the only thing that'll need to be hardcoded/duplicated from Feliz is the list of type names inside the prop module (only the actual type names).

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 10, 2019

Okay, I made it! 0.4.0 is in the pipeline.

Even just aliasing all inherited enum prop types (e.g. prop.role, prop.transform), including the documentation for each type, added more then 5500 lines to the props file. So I'm glad I got this working.

Feliz.MaterialUI must then be updated each time a new enum prop type (a new type in the prop module) is added to Feliz.

I chose to alias withType as type' to avoid naming conflicts with potential new MUI props called withType (and to be consistent with the rest of the Feliz.MaterialUI API).

Give it a try 😃

Thanks for pushing me on this until I was too nerd-sniped to give it up! 😆

@cmeeren
Copy link
Collaborator

cmeeren commented Oct 10, 2019

Maaan, now even F# is working against me. It seems I am pushing type aliasing past its limits. 😒

The code is correct and works sometimes (e.g. from the same file, or with project references), but other times (e.g. from another file, or from the 0.4.0 NuGet package) certain random props aren't available.

I think this puts an end to all attempts at "inheritance" for static types using type aliasing. There are four options left:

Again, I have not entirely warmed to duplicating arbitrary (or all) base API props.

I have unlisted the 0.4.0 NuGet package.

cmeeren added a commit that referenced this issue Oct 10, 2019
 - Disable native prop inheritance
 - Duplicate inherited MUI props in child components
 - More info: #20 (comment)
@cmeeren
Copy link
Collaborator

cmeeren commented Oct 10, 2019

0.5.0 is in the pipeline with MUI prop inheritance (duplication), but no native DOM prop duplication. That should be done on a component-by-component and prop-by-prop basis as desired, if at all. I have no immediate plans of adding native DOM props, but I'm open to suggestions.

@Zaid-Ajaj
Copy link
Author

I am gonna give 0.5.0 a try very soon and let you know of my findings! Thanks a lot for keeping an open mind for the possible options out there

@Zaid-Ajaj
Copy link
Author

This also means I need to test my packages often with the released packages as well, not just the project-referenced ones only

@Zaid-Ajaj
Copy link
Author

Also I loved the nerd-sniping reference ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants