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 :include_blank option to Field::Select #2018

Merged
merged 1 commit into from
Jul 24, 2021
Merged

Conversation

efatsi
Copy link
Contributor

@efatsi efatsi commented Jul 14, 2021

  • copy/pasted pretty directly from Field::BelongsTo
  • defaults to true similar to BelongsTo configuration

@efatsi efatsi force-pushed the main branch 2 times, most recently from 4991c34 to 809fbf1 Compare July 14, 2021 17:46
@efatsi
Copy link
Contributor Author

efatsi commented Jul 19, 2021

Had a thought: perhaps a better move would be to set the default to false for the new Fields::Select#include_blank option. That would mean this PR is not a breaking change, and users could opt-in to including a blank option.

Anyone on the maintenance team have an opinion here?

@pablobm
Copy link
Collaborator

pablobm commented Jul 22, 2021

Thank you for this @efatsi. I agree that we want to avoid unexpected changes of behaviour, and therefore the default should be the same as it is now. Would you be able to make this change?

@efatsi
Copy link
Contributor Author

efatsi commented Jul 22, 2021

Can do!

- copy/pasted pretty directly from Field::BelongsTo,
  except it defaults to false
@efatsi
Copy link
Contributor Author

efatsi commented Jul 22, 2021

@pablobm just pushed an update. Looks like ruby-25 failed on Circle, although the failure seems unrelated to the changes I've made.

Copy link
Collaborator

@pablobm pablobm left a comment

Choose a reason for hiding this comment

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

I think I have figured out the issue with CI (see #2023). Let me double-check it works, merge the fix, and I'll merge this.

@pablobm pablobm merged commit 3e24d95 into thoughtbot:main Jul 24, 2021
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.

2 participants