Shmew/Feliz.MaterialUI

Popover anchorEl uses Element instead of HTMLElement

Closed this issue · 7 comments

I think this relates to issue #16. When trying to create a React component the Feliz.prop.ref is returning an HTMLElement option, but popover.anchorEl wants Element option.

The two work arounds I have are to either cast:
popover.anchorEl (anchorRef.current |> Option.map (fun (el: HTMLElement) -> el :> Element))

or make my own attribute
Interop.mkAttr "anchorEl" anchorRef.current

You could do Option.map unbox for a shorter version. Or, since this is Fable, you can use this IIRC if you know it'll work at runtime.

popover.anchorEl !!anchorRef.current

I don't see this as a bug. F# requires explicit upcasts in some cases.

I consider it as a missing overload. It makes it difficult to reproduce the examples from the Material UI documentation. I understand that there can't be a perfect match, but most of my time spent on the front end is navigating these little gotchas. I don't have the experience however to judge whether these are real pain points in the library or if I'm just not knowledgeable enough about the F# and Fable.

As an example, I'm using a popover because I wanted it to show up below the button. At first I attempted this:

Mui.menu [
    menu.anchorEl state.Anchor
    menu.open' state.IsOpen
    menu.anchorOrigin.bottomLeft
    prop.children [
        Mui.menuItem [ prop.text "A" ]
        Mui.menuItem [ prop.text "B" ]
        Mui.menuItem [ prop.text "C" ]
    ]
]

But this threw an error message "You can not change the default anchorOrigin.vertical value when also providing the getContentAnchorEl prop to the popover component. Only use one of the two props." I tried menu.getContentAnchorEl null which didn't compile. This pushed me down the route of using a popover directly.

Another example is the tablePagination.onChangeRowsPerPage handler. The example JS code uses setRowsPerPage(+event.target.value). The value property is not accessible on the Browser.Types.Event that gets passed to the handler. To get the value required me to open Fable.React to gain access to the Value extension method. But Value returns a string while the actual underlying type is a number. I finally solved this with tablePagination.onChangeRowsPerPage (fun ev ->dispatch (PageSizeChanged (unbox<int> ev.Value))).

Don't get me wrong, I am extremely grateful for the work you've done putting this library together. I just struggle to make it work sometimes.

I'll fix it. It seems that it's solved by replacing

anchorEl (value: Element option)

with

anchorEl (value: #Element option)

in the definition of the prop.

I'll have a look at the other ones you mentioned, too.

Released in v1.2.6.

Thank you. Can you explain what the # does in the prop definition? I'd like to get to the point to where I can submit a PR when I hit an issue.

Can you explain what the # does in the prop definition?

Does this SO answer explain it?

I'd like to get to the point to where I can submit a PR when I hit an issue.

I appreciate that. However, this library is "extra complicated" in that way, because you can't just change the actual prop code; the prop code is generated by a source generator, so you have to change stuff in the generator. I have never prioritized cleaning this code up properly after the initial experimentation, which means it's fairly messy, which means I kind of have a system that probably isn't too easy for others to get into, and I'm picky about keeping that "system" so I can continue to understand and maintain the code.

If you want, knock yourself out, but I suggest always opening an issue first anyway. :)

@Shmew Were you able to look at the tablePagination.onChangeRowsPerPage? Should we maybe open a new issue?
I had the same problem as Bennie with this Event.