[docs] Inherited props are not documented

[leesei]

Many components uses EnhancedButton and hence support onTouchTap() attribute (by passing through {...others})
onTouchTap() is crucial to handling click/tap for buttons, however this was not documented in:

• Flat Button
• Raised Button
• Floating Action Button
• Icon Button

Same with:

• DatePicker

Is this intentional?

[leesei]

Alas, I've just found out that onTouchTap() was provided by zilverline/react-tap-event-plugin.
But now with the move to include it internally (#3068/#3079), we should update the doc to state that onTouchTap() is available on all components.

[newoga]

@leesei Yeah honestly I can't think of a good reason why it isn't already documented. It's even documented in other components. Even though we're investigating including it in #3068 and #3079, the react-tap-event-plugin is a prerequisite for material-ui. Feel free to open up a PR to add the prop for us! 😄

Thanks!

[alitaheri]

It's because it's one of the props that are forwarded. We really have to document those props that are passed down with {...other}! something like: inherited props.

[newoga]

Oh, you mean to passed to <EnhancedButton />. I see. Yeah I wonder what the best approach to documenting that is. That would be useful information in general.

[leesei]

I would like to know if it is desirable to hide EnhancedButton from the user?
If so we have to copy and paste those interited props to every button.

Otherwise it is better to add EnhancedButton to the doc and specify its props, and refer to it in the users' doc.

[alitaheri]

@leesei That also works, except that some of the props are intercepted. so not all props are passed down, and not all props are inherited from EnhancedButton. This needs a lots of thought. I have no idea how we can achieve this 😅 😅

[leesei]

Then it's better to manually trace the props for each class and document what props will eventually be handled.
That's indeed a daunting task.

---

Or we can still add doc for EnhancedButton and also document in the user class what is passed down.

[alitaheri]

@leesei That can also work, but exposing internal components scares me O.o Although it might not be a bad idea to have an internal section for this purpose and tell users not to directly use these components O.o

@oliviertassinari @newoga @mbrookes I would like to hear your insight on this too. thanks ^_^

[leesei]

My philosophy too is to hide implementation details. Library users have a tendency to abuse them if there is an easy way to get that info.

[alitaheri]

True that 😅 😅

[mbrookes]

There's a related issue: #2940

I don't know the right answer - @alitaheri hit the nail on the head: #3172 (comment)

So just exposing EnhancedButton in the docs doesn't solve the problem.

[oliviertassinari]

If so we have to copy and paste those interited props to every button.

I feel like this is our best option. We should only document our public API, we can't predict how properties will be passed down to internal components.

There's a related issue: #2940

That's a really interesting problem. The Date Picker, is special. We are not only exposing DatePicker but also DatePickerDialog. People can built their own DatePicker with this second component. Same here, I think that we should add the default value of properties to the DatePicker component.

[alitaheri]

I think what @oliviertassinari is proposing is our best option 👍 👍

[mbrookes]

So also related to #2973 which must be done manually. @heetvachhani has started on that one.

I feel an umbrella issue coming on. 😄

[mbrookes]

Include EnhnacedButton containerElement in this. #850 / #2982.

[tintin1343]

@mbrookes : Have we reached an understanding about not exposing the internal components in the docs? Can we close this?

[mbrookes]

We do still need to document inherited props, but it's a manual effort, and hasn't been tackled yet.

We also sort of have the opposite problem, in that we document some event props that bubble up from underlying components / elements, for example onKeyboardFocus, onMouseEnter, onMouseLeave etc.

We typically only document the ones we have intercepted (e.g. to set style related state), and passed up, so the list of props documented on any particular component varies, even though the full list of DOM events should be available.

For Chip, I moved those event prop descriptions out of the main list of properties, into 'Additional properties', so that the main list is less cluttered, with a view to doing the same for other components.

However I'm now wondering if we should be documenting those props at all (it's DOM documentation not MUI documentation), or if so, whether we should just document the most useful ones in one place, and link to that from the props docs.

This would reduce the clutter and repetition across the component prop docs.

[oliviertassinari]

It's DOM documentation, not MUI documentation

You raised a good point! I like this idea. What's the point of documenting standard DOM properties?
Regarding the most useful DOM properties, I see three of them:

• onClick
• style
• className

I like the idea of not documenting them at all. Still having them working and tested. We could remove them from the docs site and keep them from the PropTypes if we override them.

[ngbrown]

It's DOM documentation, not MUI documentation

While the DOM properties fall through to an underlying element much of the time, components don't always apply ...other to the underlying element, and some properties are overridden and not merged with any passed properties at all. The only way to know for sure is to look at the source code.

Personally, I would prefer that all relevant DOM properties get documented on each component, not just the ones that are intercepted. Maybe it should be in an "inherited properties" section of the documentation, but showing that a FlatButton or a StepButton takes both onClick and onTouchTap should be part of the documentation. If there's a reason to not have onClick in the list, than leave it off, but right now they have neither.

[mbrookes]

The only way to know for sure is to look at the source code.

Obviously if there are inconsistencies, they would either need to be fixed, or, if intentional, documented.

I would prefer that all relevant DOM properties get documented on each component

That would be one crazy big list, you wouldn't be able to see the wood for the trees. (Type window.HTMLDivElement.prototype in your browser console if you're curious.)

[avaragado]

As a total newbie with material-ui (literally day one) I've hit this.

My newbie goal was to create an AppBar with a couple of links in, as a simple test of how easy the components are to work with. Things that surprised me:

• The documentation for FlatButton doesn't mention tap or click handlers, either in the Properties list or the prose or the examples. onClick and onTouchTap are mentioned in the installation section, but the main components doc is reference material so should be exhaustive, even if it just refers to another page for inherited props.
• FlatButton includes an href property, but most SPAs use something like react-router, which has a Link element for linking to app routes. How do I use FlatButton with Link? It's not mentioned, which is a shame for such a common use case. From issue Button wrapped with Link from react-router does not work in Safari #850 it seems there's an undocumented containerElement property I can use for this. As it's undocumented, I can't tell if it's considered private and thus dangerous to use.
• Slightly off-topic, but AppBar seems pretty limited. I'd like to see some advice on how to implement common use cases like "more than one button on the bar". Right now I've got something working by placing a div inside iconElementRight that contains some FlatButton elements with manual style overrides (colour, positioning). I'm unsure whether this is recommended or discouraged. When I think about implementing an app bar such as the one on materialup.com, I wonder whether I'd be better off growing my own component using flex, or trying to persuade/coerce/subvert AppBar to do it.

Those are my day one observations: I hope they're useful. Thanks!

[abramz]

This was opened over a year ago, what is holding up documenting your APIs?

[mbrookes]

@abramz This is an open source project. If you want to help, ask for guidance and get stuck in.

Here's the umbrella issue: #3191

Incidentally, core team focus has move to the next branch.

[huangming1994]

@leesei In fact, ListItem also.

[atrauzzi]

Any chance the TypeScript definitions could be updated to reflect all this as well?

[oliviertassinari]

@atrauzzi As far as I know, they already are on v1-beta.

[atrauzzi]

As I mention in the other ticket, I'm not sure that's of any help until it's out of beta. For those of us using 0.x, we're stuck with a false error that won't go away.

[abramz]

Yah, it sucks but they have been clear they are not working on 0.x issues because they trust themselves to fix everything single issue in 1.0 that is brought up in the meantime...even though they close the issues.

…

On Sat, Dec 30, 2017, 9:44 PM Alexander Trauzzi ***@***.***> wrote: As I mention in the other ticket, I'm not sure that's of any help until it's out of beta. For those of us using 0.x, we're stuck with a false error that won't go away. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3172 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD4v2Yn7hERSonRR8QlMTP0-XNdQ1FVAks5tFx86gaJpZM4HTI1Q> .

[mbrookes]

@atrauzzi You're welcome to submit a PR.

@abramz there is no "they", it's just "us", the MUI community. That includes you. If something isn't working for you, get stuck in and fix it.

[jedwards1211]

@avaragado I understand what you mean, but event handler props like onClick, onTouchTap etc. are supported by React itself on DOM elements, and I think the docs just need to clarify that additional properties get passed down, and event handlers will will wind up on the appropriate DOM element. If the dozens of different event handlers were documented on every component, they would get in the way of finding prop documentation that's specific to a component.

@mbrookes I think @avaragado has a good point react-router Links though; I think it would help a lot of people if components like Button and ListItem that are commonly used with Link included at least a link from their documentation pages to https://material-ui.com/demos/buttons/#third-party-routing-library. Would you be open to a PR for that?

[eps1lon]

@jedwards1211 The API pages already link to demo pages. So we actually have a link from the Button API page to the Button demo with react-router usage.

Also: The search is actually pretty good. Type react-router in the search bar and the first result is https://material-ui.com/getting-started/faq/#how-do-i-use-react-router which is pretty unambiguous if you ask me.

The only thing I'm missing is that google displays the anchor links in the search result.

[jedwards1211]

@eps1lon I understand that, but the idea to search for react-router in the docs page has to pop into someone's head, and we can guide them better than leaving it up to whether they think to search for this. So I am trying to advocate for putting additional links in any places people are likely to find them. Since a lot of people will be looking through the API pages, and some of them will simultaneously be wondering how to use a given component with React Router, I think there are only benefits and no drawbacks if API pages for Button, ListItem, etc. also have links to https://material-ui.com/getting-started/faq/#how-do-i-use-react-router. (That or just the faq on how to use any third-party routing library.) As I said, I am offering to make this PR if you are likely to merge it.

[eps1lon]

You can go ahead and open a PR so that I can understand where a link is currently missing. Just want to note that linking to the FAQ from the API is not very helpful since the user would still have to click another link to the demo page that is already linked from the API. The API pages should only be for interface docs. Usage should be located in the demos.

As I said, I am offering to make this PR if you are likely to merge it.

I don't dictate what is merged or not. This is a team effort. A PR is helpful here over theorizing it because it provides a deploy preview. Discussing this in a closed and somewhat unrelated issue doesn't help its visibility.

[oliviertassinari]

The search is actually pretty good.

This is how 80% of the people should be able to find the content they are looking for: Algolia. If it's working, then we are optimizing for the 20%, it's not important. Still, we can always improve it. It's also important to consider that too much documentation can harm as much as too little.

[jedwards1211]

Just want to note that linking to the FAQ from the API is not very helpful since the user would still have to click another link to the demo page that is already linked from the API.

I was talking about keeping the link in the demo page and adding the same link somewhere to the API page, for instance in the description of href properties.

A PR is helpful here over theorizing it because it provides a deploy preview

I ask if something is likely to get merged to protect my valuable time, because I've had a PR I worked hard on here and refined during repeated discussion arbitrarily gutted into something that completely missed the original point of the PR at the last minute, and I really regretted how much time I had spent on it.

[oliviertassinari]

@jedwards1211 Do you plan on updating the API of material-ui-popup-state to support hooks? :)

[jedwards1211]

@oliviertassinari Yes, I would like to, once hooks are out of alpha/beta stage