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

Implement currying interface, resolve #18 #19

Merged
merged 2 commits into from
May 9, 2017
Merged

Implement currying interface, resolve #18 #19

merged 2 commits into from
May 9, 2017

Conversation

lttb
Copy link
Member

@lttb lttb commented May 9, 2017

BREAKING CHANGE: keep just only curried interface

@lttb lttb requested a review from kof May 9, 2017 11:05
@kof
Copy link
Member

kof commented May 9, 2017

The question is, should we possible have just one interface?

@kof
Copy link
Member

kof commented May 9, 2017

It would make the lib code simpler and users code more unified, less confusion.

@kof
Copy link
Member

kof commented May 9, 2017

I did the exact same thing for react-jss some time ago.

@lttb
Copy link
Member Author

lttb commented May 9, 2017

It would make the lib code simpler and users code more unified, less confusion.

I agree, looks like it's better to make this breaking change now

@lttb
Copy link
Member Author

lttb commented May 9, 2017

And I can't get real pros for the previous API

fontSize: 12,
color: (props) => props.theme.textColor
})

// You can also use curried interface this way.
Copy link
Member

Choose a reason for hiding this comment

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

its all curried ...

Copy link
Member

Choose a reason for hiding this comment

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

but yeah a separate example is still good.

@kof
Copy link
Member

kof commented May 9, 2017

btw. don't forget to add changelog with breaking change notice and release a new major verison

@lttb lttb merged commit c3f6f11 into master May 9, 2017
@lttb lttb deleted the feature/curry branch May 9, 2017 16:28
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 92.857% when pulling 531c927 on feature/curry into e80e66a on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 92.708% when pulling dbfdaa3 on feature/curry into e80e66a on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 92.708% when pulling 9724370 on feature/curry into e80e66a on master.

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

3 participants