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] Fix misc typos, grammar and add minor clarifications #9112

Merged
merged 2 commits into from Nov 13, 2017
Merged

[docs] Fix misc typos, grammar and add minor clarifications #9112

merged 2 commits into from Nov 13, 2017

Conversation

mbrookes
Copy link
Member

@oliviertassinari It's been a while, so I have given the (non-demo / non-API) docs a cover-to-cover read, and made a few tweaks. The only real addition is in overrides.md, so please take a look and make sure it's technically correct.

We seem to have a bit of a mix of hard-wrapped, not wrapped at all, and variable-line-length. Other than in overrides, I resisted changing that, as it would make the other changes hard to follow, but I'm happy to go back and standardise this a separate commit if there's a preferred format.

Questions:

  • Should the API section move from Customization to Guides?
  • Does Flow ready need its own 2-line section, or could that go in the FAQ? (Or is it a stub, with plans to expand this section?)
  • Would Examples be better named Example Projects?
  • In "Themes", what do you mean by "Business variables?"
  • What is the "MyLink" demo demonstrating?

@oliviertassinari
Copy link
Member

Should the API section move from Customization to Guides?

I don't know. If you think it should why not :).

Does Flow ready need its own 2-line section, or could that go in the FAQ? (Or is it a stub, with plans to expand this section?)

I'm hoping we can add more information on this section.

Would Examples be better named Example Projects?

Why not.

In "Themes", what do you mean by "Business variables?"

It's some variables that are specific to the UI you are implementing. For instance, the color of a failing build on Argos-CI.

What is the "MyLink" demo demonstrating?

It's demonstrating how to use our styling solution.

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Nov 12, 2017
@mbrookes
Copy link
Member Author

Should the API section move from Customization to Guides?

I don't know. If you think it should why not :).

Just that it doesn't really seem to have anything to do with customization, so it's either Getting Started, or Guides. While it's good background to have, it doesn't feel like necessary info to have to get started, ergo Guides. 😄

Would Examples be better named Example Projects?

Why not.

Okay, cool. But... I tried updating the title, renaming the md file, and changing the route (and restarting) but I'm getting a 404. Is there a trick to it? 😕

It's some variables that are specific to the UI you are implementing. For instance, the color of a failing build on Argos-CI.

So, "Custom styles"? I guess modifications to the default theme is also customization, but these are styles that are not part of the default theme, right? (Or is this a "Brand" thing?) I just didn't grok the "business" part.

What is the "MyLink" demo demonstrating?

It's demonstrating how to use our styling solution.

Don't we have that already here?

Looking again, it seems like it's basically the same code, other than adding a couple of unused props. I was originally confused because it doesn't come with any introduction or explanation. Now I'm even more convinced it's redundant.

Thoughts?

@oliviertassinari
Copy link
Member

it doesn't feel like necessary info to have to get started, ergo Guides.

Sounds good.

Okay, cool. But... I tried updating the title, renaming the md file, and changing the route (and restarting) but I'm getting a 404. Is there a trick to it?

next.js use the filename in /pages/x as the pathname /x.

So, "Custom styles"?

Sounds good.

Don't we have that already here?

Oh, sorry. It's not the same demo. The first one is using withStyles() of Material-UI while the other one is using injectStyles() of react-jss.

@mbrookes
Copy link
Member Author

next.js use the filename in /pages/x as the pathname /x.

I'm sure that's what I did, but I'll try again.

Oh, sorry. It's not the same demo. The first one is using withStyles() of Material-UI while the other one is using injectStyles() of react-jss.

So it's showing the interoperability of native jss injectStyles() in user space with jss in Material-UI?

Is the paragraph above that demo about Material-UI using a fork still valid? It seems we're using the jss npm package.

@mbrookes
Copy link
Member Author

mbrookes commented Nov 13, 2017

@oliviertassinari I tried moving api.md from customization to guides, and updated withRoot.js to add:

      {
        pathname: '/guides/api',
        title: 'API',
      },

to /guides children array (and removing the equivalent from /customization), but even after restarting, I get a 404 (the same problem I had with renaming examples.md to example-projects.md). Is there another step that I"m missing?

@oliviertassinari
Copy link
Member

I'm sure that's what I did, but I'll try again.

Let me have a look.

Is the paragraph above that demo about Material-UI using a fork still valid? It seems we're using the jss npm package.

This is a new section (less than a month old). Yes, it's still valid. We haven't forked jss, only react-jss.

@mbrookes
Copy link
Member Author

We haven't forked jss, only react-jss.

Sorry, I meant, react-jss. The reason I asked is that in package.json dependencies we have: "react-jss": "^7.2.0", - no mention of a fork or local package.

@oliviertassinari
Copy link
Member

@mbrookes Only some modules have been forked, not all the package.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I'm gonna merge this PR as I want to ship a new beta release by this evening.

@oliviertassinari oliviertassinari merged commit 68b3f34 into mui:v1-beta Nov 13, 2017
@mbrookes
Copy link
Member Author

Not yet, I've another batch to include.

@oliviertassinari
Copy link
Member

Ohhh 😬

@mbrookes
Copy link
Member Author

Okay, I'll do another PR.

@oliviertassinari
Copy link
Member

Ok, I'm gonna hold on the release, it's time to upgrade the dependencies.

@oliviertassinari oliviertassinari changed the title [docs] Fix misc typos, grammar and add minor clarifications. [docs] Fix misc typos, grammar and add minor clarifications Nov 13, 2017
the-noob pushed a commit to the-noob/material-ui that referenced this pull request Nov 17, 2017
* [docs] Fix misc typos, grammar and add minor clarifications.

* rename
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

2 participants