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

Potentially confusing definition of "Higher-order component" in v1 docs #9226

Closed
1 task done
michaeljonathan opened this issue Nov 19, 2017 · 10 comments
Closed
1 task done
Labels
docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. v1.x

Comments

@michaeljonathan
Copy link

  • I have searched the issues of this repository and believe that this is not a duplicate.

Hi @oliviertassinari,

Thank you for writing this library! It's definitely one of my favorite sources for learning how to write React components.

Minor question - I'm reading the CSS-in-JS section of the v1 docs, and correct me if I'm wrong, I think I see 2 definitions of "higher-order component"...

Definition 1

We use the withStyles higher-order component to inject an array of styles...

Definition 2

API
withStyles(styles, [options]) => Higher-order Component

If I understood this correctly, the docs are saying that withStyles is a HOC that returns a HOC?

I think the React docs on HOCs seem to prefer the 1st definition; preferring instead to call the returned object an EnhancedComponent.

I'm not too worried about this personally, but I think this part of the docs may be confusing for those unfamiliar with the HOC term.

<aside>I don't personally resonate with the React official definition... I think it's confusing that React Components can be rendered but Higher-order Components can't; but this definition seems to be standard now so I've gotten used to it.</aside>

Thoughts?

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 19, 2017

If I understood this correctly, the docs are saying that withStyles is a HOC that returns a HOC?

@michaeljonathan withStyles is an higher-order component factory. I think that we should apply the following change to make it less confusing:

-We use the `withStyles` higher-order component to inject an array of styles...
+We use the `withStyles()` higher-order component to inject an array of styles...

Do you want to submit a pull-request?

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. labels Nov 19, 2017
@leMaik
Copy link
Member

leMaik commented Nov 19, 2017

@oliviertassinari Imho, for newbies, that change doesn't make it less confusing at all. 😄

What about this:

-We use the `withStyles` higher-order component to inject an array of styles...
+We use the higher-order component created by `withStyles()` to inject an array of styles...

@oliviertassinari
Copy link
Member

@leMaik Sounds way better ⛩

@pajaydev
Copy link
Contributor

Can I make a PR?? @oliviertassinari.

@pajaydev
Copy link
Contributor

submitted the pull request #9235

@rosskevin
Copy link
Member

Perhaps I'm confused, but to me withStyles is the HOC. I guess you could say withStyles creates the HOC, but most would regard withStyles as the HOC itself.

@pajaydev
Copy link
Contributor

ohh okay got it. @oliviertassinari Can I revert the changes??.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 20, 2017

Perhaps I'm confused, but to me withStyles is the HOC.

@rosskevin Not if we refer to other libraries. Let's take the recompose documentation: https://github.com/acdlite/recompose/blob/master/docs/API.md.

a higher-order component (HOC) refers to a function that accepts a single React component and returns a new React component.

Most Recompose helpers are functions that return higher-order components:

I think that keeping the distinction will help people better understanding the concepts.

@leMaik
Copy link
Member

leMaik commented Nov 20, 2017

I think that keeping the distinction will help people better understanding the concepts.

This! 👍 withStyles is just a plain old function, nothing special about it. It returns a HOC, which is special

@mbrookes
Copy link
Member

mbrookes commented Nov 21, 2017

So it seems that pretty much everyone agrees with this definition of a higher-order component:

a higher-order component (HOC) [is not actually a component, but rather] refers to a function that accepts a single React component and returns a new React component.

@rosskevin:

most would regard withStyles as the HOC itself

I have to admit I thought of it that way, and apparently so did it's author? :

https://github.com/mui-org/material-ui/blob/65532516937928110a28c05db1c1067f089d571e/src/styles/withStyles.js#L101-L103

But I agree that making the distinction is worthwhile, as it also explains the presence of the IFFE syntax that is perhaps unfamiliar to less experienced devs. (I recall having a "WTF?" moment the first time I saw function()()!)

If we fix it in the docs, we should probably fix the comment too, and also here: https://github.com/mui-org/material-ui/blob/v1-beta/docs/src/pages/getting-started/frequently-asked-questions.md#how-do-i-combine-the-withstyles-and-withtheme-hocs

Edit: Also here: https://material-ui.com/customization/overrides/#overriding-with-class-names

Secondly, I may have added to the docs confusion by including the words "We use the withStyles Higher-order Component to inject an array of styles into the DOM as CSS".

Would it be more correct to say that the resulting enhanced component injects the CSS? (At what point is the CSS actually injected?)

Since they're all related it would be good pick them up in the associated PR.

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 good first issue Great for first contributions. Enable to learn the contribution process. v1.x
Projects
None yet
Development

No branches or pull requests

6 participants