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

Add an optional emptyValue to SelectInput #2780

Merged
merged 3 commits into from
Mar 8, 2019
Merged

Conversation

edy
Copy link
Contributor

@edy edy commented Jan 17, 2019

#2234 introduced a breaking change for our application. This change adds an optional custom value which is used when allowEmpty is set.

Previously null was set as an empty value, now you can set null again.

marmelab#2234 introduced a breaking change for our application. This change adds an optional custom empty value which is used when allowEmpty is set.
@sghribi
Copy link

sghribi commented Feb 6, 2019

Thanks @edy for this PR!

Is it ok to be merged for 2.7.1?

@edy
Copy link
Contributor Author

edy commented Feb 6, 2019

sure

@edy
Copy link
Contributor Author

edy commented Feb 16, 2019

Is it ok to be merged for 2.7.1?

would you merge this for 2.7.2?

@sghribi
Copy link

sghribi commented Mar 4, 2019

Hi @djhi @fzaninotto

Could you review/validate this PR?

Thanks!

@djhi
Copy link
Collaborator

djhi commented Mar 4, 2019

Sorry @edy, as it is a new prop, can you please target the next branch?

@edy edy changed the base branch from master to next March 4, 2019 16:59
@edy
Copy link
Contributor Author

edy commented Mar 4, 2019

Done.

Why isn't it possible to merge into master? All tests pass and the change is backward compatible too.

@djhi
Copy link
Collaborator

djhi commented Mar 4, 2019

As I said in previous comment, this adds a new prop, hence a new feature. If we do merge it, it will be on the next branch for the 2.8 release (semver ?)

Copy link
Contributor

@Kmaschta Kmaschta left a comment

Choose a reason for hiding this comment

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

Seems ok to me 👍

Just one thing: mind you document this new prop in the related doc?
https://github.com/marmelab/react-admin/blob/master/docs/Inputs.md#selectinput

@edy
Copy link
Contributor Author

edy commented Mar 4, 2019

also done

@fzaninotto fzaninotto merged commit adc653c into marmelab:next Mar 8, 2019
@fzaninotto
Copy link
Member

Thanks!

@fzaninotto fzaninotto added this to the 2.8.0 milestone Mar 8, 2019
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.

5 participants