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] migrate select-field to the new standard #2694

Merged
merged 1 commit into from
Dec 30, 2015

Conversation

alitaheri
Copy link
Member

Ok it's ready to review.

Closes #27, #2456, #2213

@oliviertassinari Take a look.

@alitaheri alitaheri added docs Improvements or additions to the documentation PR: Needs Review labels Dec 29, 2015
@alitaheri alitaheri force-pushed the docs-codgen-select-field branch from 12e27ec to 6a7adf9 Compare December 29, 2015 15:05
import SelectField from 'material-ui/lib/SelectField';
import MenuItem from 'material-ui/lib/menus/menu-item';

export default class SelectFieldCustomLabelExample extends React.Component {
Copy link
Member

Choose a reason for hiding this comment

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

It should match the name of the file. I would put example before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then it would be inconsistent with the requires.

Copy link
Member

Choose a reason for hiding this comment

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

Then we should also invert the ordre in the import 😁.
I think that it's better to be more end more specific in the name.

Copy link
Member Author

Choose a reason for hiding this comment

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

how about an exact match: ExampleCustomLabel and also in the Page component again, the exact name: ExampleCustomLabel?

Copy link
Member Author

Choose a reason for hiding this comment

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

@oliviertassinari I think we should leave the discussion of this for when we want to move the docs to src.

Copy link
Member

Choose a reason for hiding this comment

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

I don't get it.
I think that we should use SelectFieldExampleCustomLabel.

Copy link
Member Author

Choose a reason for hiding this comment

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

I get what you mean. I'm only saying not all of the other pages follow what you say. We should change them all at once. I don't like to see some follow and some not. Besides it still wouldn't match the file name ExampleCustomLabel. I think we should have a discussion on this when we want to move these files to their corresponding component folder later on. we are still new to this way of organizing the docs.

Copy link
Member

Choose a reason for hiding this comment

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

I don't like to see some follow and some not.

I agree. I think that the first component was right:
https://github.com/callemall/material-ui/blob/master/docs/src/app/components/pages/components/AppBar/ExampleIconMenu.jsx

@alitaheri
Copy link
Member Author

@oliviertassinari Is this alright to merge?

/**
* The style object to use to override the `DropDownMenu`.
*/
selectFieldRoot: React.PropTypes.object,
Copy link
Member

Choose a reason for hiding this comment

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

We will have to do something about this property at some point. 😳

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely, I was like HOLY JESUS WHAT IS THIS!!!

we shouldn't introduce a lot of breaking changes. This has to wait a little longer -_-

@oliviertassinari
Copy link
Member

@alitaheri I have tested it, great stuff 👍.

@alitaheri alitaheri force-pushed the docs-codgen-select-field branch from 8d33d7a to 134ecd2 Compare December 29, 2015 21:14
@alitaheri
Copy link
Member Author

@oliviertassinari Thanks ^_^ I did as you asked. merge this if it's alright 😁 👍 👍

@oliviertassinari
Copy link
Member

@alitaheri A litlle rebase and we can merge 👍

@alitaheri
Copy link
Member Author

Yeah I forgot 😁 On it

@alitaheri alitaheri force-pushed the docs-codgen-select-field branch from 134ecd2 to ecaef63 Compare December 30, 2015 10:36
@alitaheri
Copy link
Member Author

@oliviertassinari All done. merge if it's all good 😁

oliviertassinari added a commit that referenced this pull request Dec 30, 2015
[Docs] migrate select-field to the new standard
@oliviertassinari oliviertassinari merged commit 49d3e8c into mui:master Dec 30, 2015
@oliviertassinari
Copy link
Member

@alitaheri Thanks!

@alitaheri alitaheri deleted the docs-codgen-select-field branch December 30, 2015 11:00
@alitaheri
Copy link
Member Author

Thank you 🎉 🎈 🎉

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.

2 participants