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

[TextField] Forward name props to the input #6469

Merged
merged 2 commits into from Apr 1, 2017
Merged

[TextField] Forward name props to the input #6469

merged 2 commits into from Apr 1, 2017

Conversation

novembrea
Copy link
Contributor

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Closes #6467.

@oliviertassinari oliviertassinari added component: text field This is the name of the generic UI component, not the React module! next and removed PR: Needs Review labels Mar 30, 2017
@oliviertassinari oliviertassinari changed the title [TextField] Forward name props to the input. [TextField] Forward name props to the input Mar 30, 2017
@novembrea
Copy link
Contributor Author

I was not sure how strictly you treat don't group multiple topics into one issue concept so I did not include relevant Component API entry update. It might be a good idea to reflect it there, though. I could do this later. For the reference, would that be okay to bundle those thing together?

@mbrookes
Copy link
Member

@nvma It's fine to include it with this PR, but not a problem if not, as it will get generate when the docs site is built.

Before you add it though, while @oliviertassinari was reviewing this, I had been looking at other components to see of we had any standard wording for the prop doc string for the case of passing props to sub-components.

I didn't find anything generally (though I didn't look at every component), and this component is all over the place. I think we should standardize on something like:

"XXX attribute of the Input element."

@oliviertassinari oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Review Accepted labels Mar 31, 2017
@novembrea
Copy link
Contributor Author

novembrea commented Mar 31, 2017

@mbrookes Thanks for clarification! Surely there's do need to PR for something that could be made automatically.
As to standardizing this usecase in docs I was under impression that @oliviertassinari thought of the name props as a sort of exception to the rule, since you guys don't intend to pass attributes directly to the input.
I myself thought about this and struggled for a couple for minutes looking at your compact descriptions of other props. I thought it would be good to explain why name prop is forwarded directly, but realised that it should be intuitive to anyone working with the framework. It was for me when I was trying to pass the name props, anyways.

@mbrookes
Copy link
Member

Surely there's do need to PR for something that could be made automatically.

Actually, that's my mistake. You can create it with npm run build:docs, but this isn't run automatically as part of the distribution build process (npm run build). If you include it with this PR, the docs will be up-to-date for anyone pulling next, if not, it'll be taken care of manually at the next alpha release.

If you could update the prop description following my suggestion, that would be great. If you could also update the other props that are forwarded, even better!

@novembrea
Copy link
Contributor Author

Yeah, I could do that, no problem. I'll update the PR.

@oliviertassinari oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Apr 1, 2017
@mbrookes mbrookes merged commit 3d45ed4 into mui:next Apr 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TextField] attributes propagation
4 participants