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

[docs] Improve wrapping docs #14351

Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jan 30, 2019

Addresses #13916 point 2

Mainly to correct the wording from "forwarding" to "hoisting" "copy over" which is more common (see hoist-non-react-statics https://reactjs.org/docs/higher-order-components.html#static-methods-must-be-copied-over).

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Jan 30, 2019
@@ -10,9 +10,10 @@ To solve this problem we tag some of our components when needed
with a `muiName` static property.

However, users like to wrap components in order to enhance them.
That can conflict with our `muiName` solution.
That can conflict with our `muiName` solution. If you wrap a component verify if
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been avoiding the use of "users" (or worse "people") in the docs (and also possessives such as "we", and "our" where possible). Good practice in product documentation is to write as if speaking to the user, so:

"However, you may need to wrap a component in order to enhance it, which can conflict with the muiName solution."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbrookes This is a great guideline. Should we write it somewhere or it's enough to have it implicit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can find a good public documentation style guide we could link to it from somewhere. I started to document conventions in the github wiki way back, but it was far from comprehensive. Better to point to a good resource than to try to reinvent the wheel.

If you encounter this issue, you need to:
1. Forward the properties.
1. Hoist these properties.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree that this is more common terminology. (So does Google if you search for it in the context of React). It also risks confusion with hoisting in the JS sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's hoisting in the js sense?

Maybe "copy over" like stated in the react docs? https://reactjs.org/docs/higher-order-components.html#static-methods-must-be-copied-over

Copy link
Member

@mbrookes mbrookes Jan 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[hoisting](https://developer.mozilla.org/en-US/docs/Glossary/Hoisting>

The React docs also talk about "passing props down", "and passing props through", so how about:

"1. Pass the child properties through the wrapper component."

or similar?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not talking about component props but static properties. This is what the linked section is talking about.

@oliviertassinari oliviertassinari merged commit 019da84 into mui:master Jan 31, 2019
@eps1lon eps1lon deleted the docs/composition-wrapping-improvement branch January 31, 2019 09:27
@mbrookes
Copy link
Member

mbrookes commented Feb 1, 2019

@eps1lon

We are not talking about component props but static properties.

I think you may have misunderstood the existing wording. I believe steps 1 and 2 were describing two different aspects of the demo: prop forwarding; and implementing the static property (albeit the demo is in the context of the latter). Now they describe one (somewhat redundantly).

That's presumably why I didn't agree with your wording, and you didn't understand why not, since we were discussing two different things.

I suggest we put 1 back how it was, but we could use "props' if "properties" is confusing in this context.

@eps1lon
Copy link
Member Author

eps1lon commented Feb 1, 2019

@mbrookes
The section starts with

In order to provide the maximum flexibility and performance, we need a way to know the nature of the child elements a component receives. To solve this problem we tag some of our components when needed with a muiName static property.

"nature of the child elements" is determined by a static property. Not by the instance props. Second sentence explicitly talks about static properties. Not instance properties.

Nothing in this section is/was describing that users need to forward props. The demo incidentally did this but the description didn't talk about how not forwarding props is a problem.

If this is indeed an issue we can add an example where intercepting a property can cause issues. Since it can be done on purpose this section might cause confusion because it currently does not explain why this is an issue.

@mbrookes
Copy link
Member

mbrookes commented Feb 1, 2019

No, "Forward the properties." was definitely talking about:

props => <Icon {...props} />

...but I can see how this following "If you encounter this issue, you need to:" could suggest that it's required to solve the muiName static property, rather than being required in general.

How about:

"If you encounter this issue, you need to use the same tag for your wrapping component that is used with the wrapped component. In addition you should forward the properties, as the parent component may need to control the wrapped components props."

(Or words to that effect.)

@eps1lon
Copy link
Member Author

eps1lon commented Feb 1, 2019

No, "Forward the properties." was definitely talking about:

I know this is discussing semantics at this point but it was clearly not "talking" about it. The code did forward properties but the text never talked about it.

In addition you should forward the properties, as the parent component may need to control the wrapped components props.

This sounds good to me.

@mbrookes
Copy link
Member

mbrookes commented Feb 1, 2019

it was clearly not "talking" about it

Okay then: '"Forward the properties." was with reference to...' (Sheesh! 😄 ).

This sounds good to me.

PR incoming...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants