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

Expose a .focus() method #47

Merged
merged 1 commit into from
Feb 6, 2016

Conversation

py-in-the-sky
Copy link
Contributor

Note: I used the npm run prepublish script; I apologize if this is not desired. I did not directly make changes to any of the lib/ files. All my direct changes are in the src/ files.

Motivation: for user-friendliness, it is sometimes helpful to programmatically put focus
on a text field or some other form field. Material-ui provides imperative methods for
some DOM manipulations that cannot be done via props (e.g., the TextField component from
material-ui provides the .focus() method to programmatically put focus on the field).
By giving access to the underlying material-ui component, developers can take advantage of
these imperative methods without this project having to know about them. For developers
using this project, using the interface that Formsy provides should be preferred, but using
.muiComponent provides an escape hatch when using props and Formsy methods fails.

@py-in-the-sky
Copy link
Contributor Author

Relevant issue: #39

@mbrookes
Copy link
Collaborator

mbrookes commented Feb 2, 2016

Mmm, I really like this, thank you!

As you say, it should be used with (extreme!) caution, but this is still a useful addition.

Please could you update the README with a note usage, and perhaps a simple example, I'll take for a test drive.

(Yes, would prefer just the source in the PR, it makes it easier to review, and keeps the release process consistent, but no worries.)

@py-in-the-sky
Copy link
Contributor Author

Sure! I'll get back to you with some notes and an example in the README tomorrow, hopefully.

@mbrookes
Copy link
Collaborator

mbrookes commented Feb 3, 2016

Thanks! 👍

@mbrookes
Copy link
Collaborator

mbrookes commented Feb 3, 2016

@py-in-the-sky - imperative methods (other than focus()) are going away in Material-UI. Do you think this feature will still be useful if we expose focus directly through formsy-material-ui? Are there other methods you might want to get to that aren't exposed through MUI props?

@py-in-the-sky
Copy link
Contributor Author

@mbrookes Thanks for the update! That makes things simple. I agree, in that case, it'll just be easier for formsy-material-ui to expose a .focus() method and take care of interacting with the underlying material-ui instance for the developer. I will submit an update soon!

@mbrookes
Copy link
Collaborator

mbrookes commented Feb 3, 2016

Thanks - appreciate it!

@py-in-the-sky
Copy link
Contributor Author

@mbrookes I haven't tested out the new commit yet, but I wanted to see what you thought about the approach first.

@py-in-the-sky
Copy link
Contributor Author

Although a memory leak in practice is unlikely, it might not be fully correct unless I add:

if (typeof c !== 'object' || typeof c.focus !== 'function') {
  this.focus = undefined;
}

@mbrookes
Copy link
Collaborator

mbrookes commented Feb 3, 2016

Thanks for following through, and for adding the detailed notes to the README.

One thing I noticed is that there's quote a bit of repetition across the components (naturally enough!). Do you think it would be worth extracting _setMuiComponentAndFocus to a shared module? It would help with maintainability too.

@py-in-the-sky
Copy link
Contributor Author

Yeah, I agree. React.createClass auto-binds this, right? In that case, it's as simple as extracting the code without having to actually change the body of the function. I'll do that and give it a test drive soon.

@py-in-the-sky
Copy link
Contributor Author

Okay, I test drove the latest. I ran all the specs in my project that uses formsy-material-ui, and they all passed. Then I ran the app in dev mode and played around with it; no problems. Please let me know if you run into any problems trying it out.

@py-in-the-sky
Copy link
Contributor Author

That wraps it up for all my thoughts on it. I just wanted to add protection against the very unlikely case of a memory leak. Let me know what you think!

@mbrookes
Copy link
Collaborator

mbrookes commented Feb 5, 2016

Fantastic work! Please could you squash, and we can get this merged. 👍

It will be present on formsy-material-ui components that
wrap material-ui components that possess a `.focus()`
method.  It will call the underlying material-ui
component's `.focus()` method.
@py-in-the-sky py-in-the-sky force-pushed the pr-access-mui-component branch from 9b9340f to 859a582 Compare February 6, 2016 08:26
@py-in-the-sky
Copy link
Contributor Author

Squashed! Thanks!

@mbrookes mbrookes changed the title Give access to underlying material-ui component via .muiComponent Expose a .focus() method Feb 6, 2016
mbrookes added a commit that referenced this pull request Feb 6, 2016
@mbrookes mbrookes merged commit 3c8010d into formsy:master Feb 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants