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

allow object for description prop #990

Merged
merged 2 commits into from
Aug 14, 2017

Conversation

mattkime
Copy link
Contributor

@yahoocla
Copy link

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

@mattkime
Copy link
Contributor Author

license is signed.

@papasmile
Copy link
Contributor

+1 for trying to match up API with doc, very nice. AFAICT, it would be safe to use object with babel-plugin-react-intl since it just dumps descriptors via JSON.stringify[1] (haven't tried it, though).

A couple suggestions that might make Maintainers happy with your PR:

  • Contribution Guide states "If you are adding new functionality, or fixing a bug, provide test coverage." [2] You could a test to test/unit/components/message.js by updating or copying 'renders a formatted message in a '[3] and passing in a description which is an object, then expect console not to report any warnings. They've done similarly in a different test in that same file [4].
  • very minor: linted, but it looks like convention is to add a space after comma inside an array.

[1] https://github.com/yahoo/babel-plugin-react-intl/blob/ed5dd0004d3a9368d335bd08c11c0b2cff832e1d/src/index.js#L208
[2] https://github.com/yahoo/react-intl/blob/master/CONTRIBUTING.md#submitting-a-pull-request
[3] https://github.com/yahoo/react-intl/blob/e749a389313641eb3230954e19a0f1fa42bc6df2/test/unit/components/message.js#L38
[4] https://github.com/yahoo/react-intl/blob/e749a389313641eb3230954e19a0f1fa42bc6df2/test/unit/components/message.js#L63

@eilinora
Copy link

+1 for getting this merged, makes testing harder with the added noise this causes. Would appreciate this being merged!

@okuryu
Copy link
Member

okuryu commented Aug 2, 2017

@papasmile Thanks for your suggestions.

  • Contribution Guide states "If you are adding new functionality, or fixing a bug, provide test coverage." [2] You could a test to test/unit/components/message.js by updating or copying 'renders a formatted message in a '[3] and passing in a description which is an object, then expect console not to report any warnings. They've done similarly in a different test in that same file [4].

Sure. PR for CONTRIBUTING.md welcome.

  • very minor: linted, but it looks like convention is to add a space after comma inside an array.

I think a space after comma in an array is a common practice in JavaScript. We also already run linting by ESLint, but lint rules change is welcome if we need a little work.

And this change looks good to me. Let's merge in this month if no concern by others.

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.

None yet

5 participants