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

feat(datepicker): Rename props and update onChange signature #84

Merged
merged 14 commits into from
Oct 25, 2019

Conversation

arthurdenner
Copy link
Owner

What kind of change does this PR introduce?
Feature. Closes #30.

What is the current behavior?
Our component has a different API from the other components from React Semantic UI.
The main differences are:

  • Components from RSUI have a prop named onChange;
  • Components from RSUI have a prop named value;
  • The signature for the onChange props on RSUI is: (event: SyntheticEvent, data: ComponentProps).

What is the new behavior?
All the points mentioned above are addressed.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged

…ents from React Semantic UI

we are now passing the event or undefined as the first argument and a data object with the props and
the new value as the second argument

BREAKING CHANGE: onDateChange signature was changed
this change is to match the API with others components from React Semantic UI

BREAKING CHANGE: onDateChange prop was renamed to onChange
this change is to match the API with others components from React Semantic UI

BREAKING CHANGE: selected prop was renamed to value
@arthurdenner arthurdenner added enhancement New feature or request breaking change Change the results in a major release labels Oct 24, 2019
@vercel
Copy link

vercel bot commented Oct 24, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/arthurdenner/react-semantic-ui-datepickers/lvjxtqwkda
🌍 Preview: In Progress

Copy link
Collaborator

@emersonlaurentino emersonlaurentino left a comment

Choose a reason for hiding this comment

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

Why is the event not passed as the first parameter in basic and range?

@arthurdenner
Copy link
Owner Author

Why is the event not passed as the first parameter in basic and range?

No reason. I just happened to add it as the second argument.

Do you think it's better to have it as the first for consistency or any other reason?

@emersonlaurentino
Copy link
Collaborator

Do you think it's better to have it as the first for consistency or any other reason?

Yes, once React Semantic UI uses this pattern.
f(event: SyntheticEvent, data: object)

@arthurdenner arthurdenner merged commit 7cbe2f3 into develop Oct 25, 2019
@arthurdenner arthurdenner deleted the feat/onChange-signature branch October 25, 2019 12:23
arthurdenner added a commit that referenced this pull request Nov 12, 2019
BREAKING CHANGE: onDateChange signature was changed
BREAKING CHANGE: onDateChange prop was renamed to onChange
BREAKING CHANGE: selected prop was renamed to value
@arthurdenner
Copy link
Owner Author

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Change the results in a major release enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

onDateChange signature inconsistent with Semantic UI React components
2 participants