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

Propagate Events #49

Merged
merged 1 commit into from
Feb 9, 2016
Merged

Propagate Events #49

merged 1 commit into from
Feb 9, 2016

Conversation

magicalcows
Copy link
Contributor

Added checks to event handlers to see if like-named handlers exist in props and if so, call them.

Closes #27.

@mbrookes
Copy link
Collaborator

mbrookes commented Feb 7, 2016

Thanks, looks good! Could you add a note to the docs for this, and mention that the parameters are in the Material-UI proposed future order?

@magicalcows
Copy link
Contributor Author

How's this look?
https://github.com/magicalcows/formsy-material-ui/tree/propagate-events

I assume we'll bump versions too? So I said "as of 0.3.8"...

@magicalcows
Copy link
Contributor Author

I'll update the demo code too. Maybe use some MUI SVG or Font Icons that match the values you have the in current select demo. Have it change them out according to what's picked, just to show the usage of the new feature.

@mbrookes
Copy link
Collaborator

mbrookes commented Feb 7, 2016

How's this look?

Perfect.

I'll update the demo code too.

That would be good. Feel free to switch the wording in the select if it doesn't match icons.

@magicalcows
Copy link
Contributor Author

Ok. I ended up having to change a bit more than expected on the demo code:

  1. I added a <Paper> component wrapper with a set width... Things were not laying out properly for me otherwise.
  2. I had to include MenuItem into the script, but using the proper require('material-ui/lib/menus/menu-item') resulted in ref errors when changing the value of the select. The only way I could get it to work was to include it using the const { MenuItem } = require('material-ui') method. So I opted to include all 3 components from Material UI that way.
  3. To keep the main render method clean, I created a renderSlectIcon method and have the logic for that there.
  4. I added the import for React and then added the module.exports line at the bottom. This way folks can copy & paste this directly into an existing project and try it (that's exactly how I built it, just added a new route to react-router on a project I'm working on.)

I honestly think this muddies up the example code a bit. We could think about including an example folder with a demo project that can be fired up locally. A simple webpack dev server would work... Then we can put more advanced code in there and keep the docs on the README a bit more clear. Of course, that would take more time, so it might be good to go with this and look to more extended examples in the future. It's your call though. =)

https://github.com/magicalcows/formsy-material-ui/tree/propagate-events

@mbrookes
Copy link
Collaborator

mbrookes commented Feb 7, 2016

Yes, I was trying to keep the example code simple.

If someone wants a fully working example, there's always formsy-mui-demo (linked from the examples section in the Readme). It needs to be updated though.

It happens to use Meteor, to make the build and hosting easy, but it's not using any Meteor specific code.

@magicalcows
Copy link
Contributor Author

Cool. I reverted the readme.

The problem with the meteor thing is it's not as easy for folks to look at source files. I love repos like material-ui's where the source for their docs is right there with the library, letting me poke around to see how the "experts" are doing it. ;) Their examples folder is nice too, ready to go projects to start building on (though they should probably be moved to their own repos and linked in, so they can be treated as "seeds".)

I'd be happy to help you setup an examples folder. It could even be the meteor app, though I think that's a bit heavy for a demo. A copy & paste from material-ui's examples folder and some editing could get the job done quickly! =)

@mbrookes
Copy link
Collaborator

mbrookes commented Feb 7, 2016

The problem with the meteor thing is it's not as easy for folks to look at source files.

It's one file! 😆 https://github.com/mbrookes/formsy-mui-demo/blob/master/Form.jsx

The css is empty, the lib and packages directories can go away with Meteor 1.3 (beta).

I love repos like material-ui's where the source for their docs is right there with the library

Their examples folder is nice too, ready to go projects to start building on (though they should probably be moved to their own repos

Uh, you seem to be arguing with yourself here. Let me know when you two sort it out! 😉

A minimal node-only example wouldn't be a bad idea in any case.

@magicalcows
Copy link
Contributor Author

Yup, i thought about that after I posted it.

I didn't notice the repo for the demo, I'll fork it and submit a PR on it. I'll need you to push this PR & update the version so I get it when I npm i the demo.

@mbrookes
Copy link
Collaborator

mbrookes commented Feb 7, 2016

No prob, go ahead and squash, and we can get a release out.

@bitdivision
Copy link
Contributor

Any chance of a release with this soon? Thanks for sorting this one out @magicalcows

@magicalcows
Copy link
Contributor Author

Commits squashed! Your turn @mbrookes =)

@mbrookes
Copy link
Collaborator

mbrookes commented Feb 9, 2016

Mmm. I've just had another thought. (Sorry!) 😄

The other thing mentioned in that issue is that we plan to standardise on onChange for the callback prop name.

It might make sense to standardise on that here now rather than having to deprecate the old names later. It will mean a slight difference in API until Material-UI catches up, but we'll already have that with the parameter ordering, so I think we can live with it.

If you agree I can make the change as a follow up PR before release, or you could update and re-squash.

@magicalcows magicalcows force-pushed the propagate-events branch 2 times, most recently from d1fa438 to c47e27e Compare February 9, 2016 20:29
@magicalcows
Copy link
Contributor Author

I agree, but I already did that (I think... ;)

All of the handlers are using the new (event,value) format, except the select, which uses (event, value, index).

I did however notice that I messed up in the RadioGroup. I had the check in componentDidMount instead of handleValueChange. Fixed that and pushed the changes...

@magicalcows
Copy link
Contributor Author

I was just looking over the proposal again, reading through comments.

I think I see what you're suggesting now... Example: in the FormsyCheckbox component I'd change it from this.props.onCheck to this.props.onChange ?

@magicalcows
Copy link
Contributor Author

I went ahead and made those changes, so now all of them check for this.props.onChange. I also updated the README a bit, as before it said it would check props for handlers of the same name.

mbrookes added a commit that referenced this pull request Feb 9, 2016
[Core] Propagate events as onChange()
@mbrookes mbrookes merged commit bbbf5c0 into formsy:master Feb 9, 2016
@magicalcows
Copy link
Contributor Author

sweet. =)

you gonna bump the version?

@mbrookes
Copy link
Collaborator

mbrookes commented Feb 9, 2016

Released! 🎉 Thanks for this! 👍

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.

3 participants