-
-
Notifications
You must be signed in to change notification settings - Fork 327
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 support for alternative select boxes date field helpers #535
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a changelog entry for this bugfix. Great work though!
fill_in options[:from], with: DateTime.parse(datetime) | ||
else | ||
select_date(datetime, options) | ||
select_time(datetime, options) | ||
extended_options = options.merge(base_dom_id: base_dom_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this bit here. Everything else is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an optimization. I'm passing down base_dom_id
to select_date
and select_time
so they don't need to find the label and extract the select/input id again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right I've spotted it now. It's not really extending the options, that's where I got confused. It's added the existing dom id to the options hash and then adding a guard clause to the get_base_dom_from_options method.
Although I'm not sure what I'd call it though. I think this area needs a lot of TLC and fixes but for now this will do. Thanks for explanation
I'm adding another spec forcing select boxes view helper. The helper has been around since Rails 5. It's not a problem to run the test in older versions
6918456
to
409152f
Compare
@luke-hill any chance this could be released soon? we're currently pinning to a commit: alphagov/whitehall@aa2b56b |
Probably in a couple of days if nothing else comes in |
Summary
Fixes #534
Rails 7 changed default scaffold datetime helper from
datetime_select
todatetime_field
. PR #526 forces capybara helper to work only withdatetime_field
. That's fine for this gem feature specs but not for other projects.This PR adds the ability to work with both helpers.
Details
I'm adding another spec forcing select boxes to view helper. Both
datetime_select
anddatetime_field
havebeen around since Rails 5.