Conversation
| def input(record_name, method, options = {}) | ||
| raise_broken_code_error | ||
| InstanceTag.new(record_name, method, self).to_tag(options) | ||
| # # => <input id="post_title" name="post[title]" size="30" maxlength="30" type="text" value="Hello World" /> |
There was a problem hiding this comment.
I find more value in adding the maxlength attribute as well, in addition to the size.
| # # => <input id="post_title" name="post[title]" size="30" type="text" value="Hello World" /> | ||
| def input(record_name, method, options = {}) | ||
| raise_broken_code_error | ||
| InstanceTag.new(record_name, method, self).to_tag(options) |
There was a problem hiding this comment.
This was inspired by the deprecated class InstanceTag and relied on methods such as to_input_field_tag that are no longer available. Instead I'm using Tags::TextField or similars.
|
|
||
| def options_with_default | ||
| @options.tap do |options| | ||
| options["maxlength"] = DEFAULT_MAXLENGTH unless @options.key?("maxlength") |
There was a problem hiding this comment.
the render methods sets the size attribute based on the maxlength option https://github.com/rails/rails/blob/main/actionview/lib/action_view/helpers/tags/text_field.rb#L11-L18
| when :time | ||
| Tags::TimeField.new(*generic_args).render | ||
| when :boolean | ||
| Tags::CheckBox.new(@object_name, @method, @context, "1", "0", @options).render |
There was a problem hiding this comment.
In my opinion, setting the maxlength attribute for a checkbox input doesn't make much sense
| when :time | ||
| Tags::TimeField.new(*generic_args).render | ||
| when :boolean | ||
| Tags::CheckBox.new(@object_name, @method, @context, "1", "0", @options).render |
There was a problem hiding this comment.
It makes sense for this tag to be a checkbox input instead of a select, as previously https://api.rubyonrails.org/v3.1.3/classes/ActionView/Helpers/InstanceTag.html#method-i-to_boolean_select_tag
There was a problem hiding this comment.
@okcomputer93 Although the description was never updated... I don't believe we support Rails 3 anymore... Only Rails 7, 8 and onward.
6 is already dead. A train can only wait for a single most important person.
Nobody can stop a Shinkansen.
There was a problem hiding this comment.
I am working on a PR to add testing for Rails 8.0. Rails 8.0 only supports Ruby 3.2+ so I think it might be best to cut a major release for that one, and officially drop support for Ruby < 3.2 and Rails < 7.0
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new input helper that supports a wide range of form field types (text, password, integer, date, datetime-local, time, boolean) with a default maxlength of 30, and updates the accompanying tests and model stubs to cover these cases.
- Replace the old
InstanceTagAPI with a newInputBuilderthat dispatches to the appropriateTags::*component and applies defaultmaxlength. - Add and update tests in
dynamic_form_test.rbto cover new input types and themaxlengthoption. - Extend
PostandUsertest stubs and theircontent_columnsdefinitions to include new attributes for date, time, and boolean fields.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/dynamic_form_test.rb | Extended model structs and content_columns; added tests for new field types and maxlength behavior |
| lib/action_view/helpers/dynamic_form.rb | Introduced InputBuilder and InputBuilderMethods; implemented default maxlength logic and type-based rendering |
Comments suppressed due to low confidence (2)
lib/action_view/helpers/dynamic_form.rb:18
- [nitpick] The doc comment for
inputonly shows a text field example; please expand it to document support for other types (date, datetime-local, time, boolean) and the defaultmaxlengthbehavior so readers understand all the new capabilities.
# # => <input id="post_title" name="post[title]" size="30" maxlength="30" type="text" value="Hello World" />
test/dynamic_form_test.rb:196
- Currently only the
truecase is tested for boolean inputs; consider adding a test where the attribute isfalse(or nil) to verify the hidden field and unchecked checkbox rendering.
def test_input_tag_for_boolean
| @options.tap do |options| | ||
| options["maxlength"] = DEFAULT_MAXLENGTH unless @options.key?("maxlength") | ||
| end |
There was a problem hiding this comment.
Mutating the original @options hash can introduce unexpected side effects when reused; consider duplicating (@options.dup) or merging into a new hash before setting defaults to keep the input builder stateless.
| @options.tap do |options| | |
| options["maxlength"] = DEFAULT_MAXLENGTH unless @options.key?("maxlength") | |
| end | |
| @options.merge("maxlength" => DEFAULT_MAXLENGTH) { |key, old_val, new_val| old_val } |
|
@joe-sharp Care to chime in ? or any other user of this gem ? |
I will double-check our codebase when I get back to work after this week, however I am pretty sure we don't use this functionality. So I don't have an opinion on this other than it might be nice to get this in (if it's approved) before adding testing for Rails 8.0; it's probably best we drop old versions of Ruby and Rails. |
3 participants
Add support for the
inputtag for usage asinput("post", "title").