[docs] Add some Render Props demos

[jedwards1211]

Here is what my new API idea looks like, so you can compare

(old version: #12330)

Closes #12325

[jedwards1211]

@oliviertassinari This is ready for a final review now 🎉
I documented the bindX functions exported from PopupState in the utils page.

[oliviertassinari]

@jedwards1211 Really nice tradeoff with the API. I'm sorting out the small issues I can find. Let's merge it.

[jedwards1211]

Woohoo, thanks!
If there is anyway to get docs for the bindX functions into the generated API page, let me know.

[oliviertassinari]

@jedwards1211 I'm gonna make some changes. I'm moving the component to the lab.

[jedwards1211]

Okay

[oliviertassinari]

I'm keeping iterating on this component. Well, I come to the conclusion that no matter what I try to do, I'm very close to leaking the abstraction. I'm not happy with everything I have tried, the abstraction is too restrictive, for the value provided in exchange. I'm changing the approach. I'm adding some new render prop demos. I'm gonna try the toRenderProps() method of recompose.

[oliviertassinari]

Alright, let's try that and see how that goes. No matter what, @jedwards1211 Thank you for working on it! I was really helpful to see the outcome, it's helping to make better tradeoffs :).

[jedwards1211]

@oliviertassinari what was wrong with the API I created? A week ago you were acting like you were ready to merge it as-is, now you just threw it all away without any coherent explanation of what you don't like about it. Just vague stuff about "the abstraction is too restrictive", "I'm very close to leaking the abstraction", "it's helping to make better tradeoffs". What are you even talking about? Are you trying to discourage me from making any more contributions? I feel really sad that after all the effort I made, developers are no closer to having an quick and easy way to set up menus.

Look at the WithState example side-by-side with the menu examples you had before and tell me using WithState isn't more work. You want to talk about not enough value provided in exchange, well this WithState stuff is a complete waste of extra dependencies/KB/effort.

[jedwards1211]

I guess I should just release this as a library instead so that you can see how much people are going to like the PopupState approach.

[jedwards1211]

Really nice tradeoff with the API. I'm sorting out the small issues I can find. Let's merge it.

Did you get really really stoned after you wrote this or something? This is the weirdest PR experience I have ever had

[oliviertassinari]

@jedwards1211 I understand your frustration after all the effort you have invested into this pull request! I believe it was an important step that helped making this shift, so again, thank you 🙏 .

Let me try to better explain my motivation. Yes, I eager to find an abstraction that is making the usage of the Menu / Popover / Popper simpler to use. I was seeing such potential with your suggestion. Thing started to shift after I dive into the implementation. I realized that a11y was requiring different configurations, the menu, and the popper requires different handling. So I started exploring the alternatives to solve this a11y problem. All the solutions I have tried were cumbersome. At this point, I realized that we could trade the new component for a raw WithState. Making the implementation more explicit on userland, more flexible without losing the simplicity "render props" brings and without taking the risk of making a wrong abstraction: a win-win 🚀 .

It's also important to understand that it's related to how I try to handle new APIs with the project. I would 💯 rather not abstract something unless I'm sure it's valuable, in the doubt, I would rather do nothing, wait for a clear signal-to-noise ratio (SNR). Publishing the original component as a standalone component is definitely a good path going forward to improve the SNR.

[jedwards1211]

@oliviertassinari you're talking about react-a11y right? Can you show me an example of the problems react-a11y reported that the separate bindMenu, bindPopover, and bindPopper functions were unable to solve?

My primary goal wasn't to use render props here. Maybe you thought this.state and this.setState was the problem I was trying to solve, but no, the problem I was trying to solve is that I have to remember to pass 3 or 4 props to a Menu component and 3 or 4 props to a button that triggers it each time I want to make a Menu. I don't want to pass all of those props explicitly every time I create a menu, f!@# that! It's a waste of time, too easy to make a mistake with, and too easy to create a PopupState component that keeps it DRY. I want setting up a Menu or a Popover to be a mindless process that takes as little time as possible.

In addition I want something that writes const open = Boolean(state.anchorEl) for me, as well as const openPopup = event => setState({anchorEl: event.target}) etc. These new examples haven't reduced the burden on developers to do that, so they haven't improved anything in my opinion.

The only reason I implemented PopupState as a render props component is it's the most convenient way I could think of injecting all of the boilerplate props necessary for making my life easier.

I don't think render props is worth pursuing for it's own sake. In the WithState examples, it's hardly more flexible than just using this.state somewhere, and it's:

• More typing than a this.state implementation, waaaaay more typing than PopupState
• More dependencies than a this.state implementation or PopupState
• Gonna be pretty awkward to debug compared to this.state or PopupState

Also, when it comes to a basic, super-abstract library component, I find toRenderProps(withState('anchorEl', 'updateAnchorEl', null)) really gross compared to react-powerplug's Value component.

[jedwards1211]

I do understand your concerns about signal-to-noise ratio and stability; I don't mind taking more time to discuss a new API thoroughly before merging it.

My main goal here is to get a super-easy-to-use way of creating menus and popovers into the hands of developers. If I release PopupState as a separate library, how can we put something in the material-ui docs so that all developers will find out about this separate library when learning how to create a menu or popover for the first time?

To me, if developers don't find out about the easiest way to handle menus and popover state, it's a big loss for them and we're not helping them as much as we can.

[oliviertassinari]

you're talking about react-a11y

@jedwards1211 I'm referring to the difference of how we handle accessibility between the Popover component and the Popper (aria-owns vs aria-describedby).

My primary goal wasn't to use render props here.

Oh, I'm sorry. I though it was. At least, I'm happy to have some demos with the "render props" pattern :).

It's a waste of time, too easy to make a mistake with, and too easy to create a PopupState component that keeps it DRY.

I understand the concern, I'm also eager to push this part forward. Blueprint is always an interesting source to benchmark on. They use a different strategy for the API (cloneElement): http://blueprintjs.com/docs/#core/components/popover.structure and have this nice concept of interactions: http://blueprintjs.com/docs/#core/components/popover.interactions.
So, I think that there is room for a higher-level API, this API could:

• handle a11y.
• glue the content and the target together.
• support different interactions.
• support different rendering Menu (Popover) & Popper. By the way, in the future, I would like to experiment having the Menu using the Popper over the Popover. See how to make the Popper handle the focus like the Modal is doing.

It's an ambitious task, I have some other priorities right now, but it's definitely an area I'm interested in.

If I release PopupState as a separate library, how can we put something in the material-ui docs so that all developers will find out about this separate library when learning how to create a menu or popover for the first time?

Of course!

[jedwards1211]

Okay, thanks for explaining that, and sorry I failed to notice that distinction between popper and popover, otherwise I would have already designed a solution for it. There are numerous potential solutions to the popover vs. popper issue, listed roughly in order of how much I like them:

• Bring back variant="popper"
• Separate PopperState and PopoverState components
• Separate bindPopperTrigger/bindPopoverTrigger etc. functions
• bindPopper would call some function injected by PopupState to let it know it's being used for a popper - PopupState would remember that and pass something to bindTrigger etc. to tell it to use aria-describedby instead of aria-owns
• Have the user specify whether they want to use aria-owns vs aria-describedby (it really depends on the semantic purpose they're using the component for, right?

If you're interested in using one of these solutions for a component in this library, let me know, otherwise I'll probably go ahead and release the first solution (bring back variant) as material-ui-popup-state

[jedwards1211]

Blueprint's design seems fairly convenient, but I'm skeptical of it. I try to avoid cloneElement as much as I can, though for debatable reasons (required props that get injected cause flow errors)

I think blueprint's API will be less flexible in subtle ways. For instance, what if you want two separate triggers for the same popover, mounted at different places in the app? Easy with PopupState, seems not possible with blueprint's API. At least without creating two duplicate popover elements.

[jedwards1211]

Another thing we need to consider - someone might want a hover interaction to trigger one type of popup on a component, and a click interaction to trigger another. I don't think this is possible with Blueprint's API? Unless maybe you nest one controller inside the other? that's the most concrete shortcoming of the cloneElement approach I can identify

[oliviertassinari]

@jedwards1211 Did you move the logic into its own library? I thought you had but I can't find it.

[jedwards1211]

Yeah I still need to add some examples to your docs in a PR. Here it is: https://github.com/jcoreio/material-ui-popup-state

[oliviertassinari]

thanks