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] Better definition of what withStyles is #9235

Merged
merged 1 commit into from Dec 4, 2017
Merged

[docs] Better definition of what withStyles is #9235

merged 1 commit into from Dec 4, 2017

Conversation

pajaydev
Copy link
Contributor

@pajaydev pajaydev commented Nov 20, 2017

Closes #9226.

@@ -14,8 +14,8 @@ We think that it's the future:
- [The future of component-based styling](https://medium.freecodecamp.com/css-in-javascript-the-future-of-component-based-styling-70b161a79a32)

So, you may have noticed in the demos how this *CSS-in-JS* looks.
We use the [`withStyles`](#api)
higher-order component to inject an array of styles into the DOM as CSS, using JSS. Here's an example:
We use the higher-order component created by [`withStyles`](#api)
Copy link
Member

Choose a reason for hiding this comment

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

withStyles is the higher order component. I don't think this change is correct.

Copy link
Member

Choose a reason for hiding this comment

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

I retract my earlier comment, I think this change is correct. withStyles is not an HOC, it's simply a function which takes a styles object and returns an HOC, which takes a component and returns a component.

Copy link
Member

Choose a reason for hiding this comment

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

@pelotom What about the second half of that sentence? (See the end of my #9226 (comment))

Copy link
Member

Choose a reason for hiding this comment

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

@mbrookes you mean this part?

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.

I might say something like

Calling withStyles(styles) produces a higher-order component--that is, a function which takes a component and produces a new, enhanced component. The enhancement in this case is to inject class names for the given styles into the props of the component it is applied to.

@rosskevin rosskevin added the docs Improvements or additions to the documentation label Nov 20, 2017
@oliviertassinari oliviertassinari changed the title altered definition of Higher-order component in v1 docs #9226 [docs] Better definition of what withStyles is Nov 20, 2017
@mbrookes mbrookes added the on hold There is a blocker, we need to wait label Nov 22, 2017
@oliviertassinari oliviertassinari removed the on hold There is a blocker, we need to wait label Dec 4, 2017
@oliviertassinari oliviertassinari self-assigned this Dec 4, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 4, 2017

No activity for 15 days. I think this change is going in the right direction. I'm going to merge @Ajay2507 pull-request. Thank you.

@rosskevin, @pelotom you are free to keep iterating on the topic :).

@oliviertassinari oliviertassinari merged commit cc774b4 into mui:v1-beta Dec 4, 2017
@mbrookes
Copy link
Member

mbrookes commented Dec 4, 2017

@oliviertassinari It was on hold because changing it in this one place makes it inconsistent with the rest of the docs and code comments: #9226 (comment)

Should we reopen that issue, or open a new one?

@oliviertassinari
Copy link
Member

rest of the docs and code comments

@mbrookes Oh, I was dodging the inconsistency by using withStyles for the factory and withStyles() for the higher-order Component. Maybe we can find better. @mbrookes If you think we can do better, sure!

@mbrookes
Copy link
Member

mbrookes commented Dec 4, 2017

@oliviertassinari I'll have to re-read the relevents docs with that in mind. I hadn't considered it.

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.

None yet

5 participants