-
Notifications
You must be signed in to change notification settings - Fork 356
Allow file to be -pended #751
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
Changes from all commits
599e9a3
f54ab63
92676b1
9a59f8f
b4dcd48
9f3078b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,11 +34,11 @@ def field_name(method, *methods, multiple: false, index: @options[:index]) | |
| def field_name_shim(object_name, method_name, *method_names, multiple: false, index: nil) | ||
| names = method_names.map! { |name| "[#{name}]" }.join | ||
| if object_name.blank? | ||
| "#{method_name}#{names}#{multiple ? '[]' : ''}" | ||
| "#{method_name}#{names}#{'[]' if multiple}" | ||
| elsif index | ||
| "#{object_name}[#{index}][#{method_name}]#{names}#{multiple ? '[]' : ''}" | ||
| "#{object_name}[#{index}][#{method_name}]#{names}#{'[]' if multiple}" | ||
| else | ||
| "#{object_name}[#{method_name}]#{names}#{multiple ? '[]' : ''}" | ||
| "#{object_name}[#{method_name}]#{names}#{'[]' if multiple}" | ||
|
Comment on lines
+37
to
+41
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was wondering if these were RuboCop changes, and then I saw your other e-mail. If there are extensive RuboCop changes, I suggest a separate PR, but since the scope is limited, it's fine to be here. More context: We could freeze RuboCop to certain rules, etc., but honestly I don't have time to do a proper job of maintaining this gem, let alone adding additional tasks around RuboCop, so we just fix style things as they come up.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might think about turning off the auto-correct, then. That would show the findings, and make it more deliberate to fix them in a given patch.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good suggestion. I'll look into that. I'm not even sure why auto-correct is the default, now that you mention it. |
||
| end | ||
| end | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -195,6 +195,51 @@ class BootstrapFormGroupTest < ActionView::TestCase | |
| assert_equivalent_html expected, bootstrap_form_for(@user) { |f| f.text_field :email, prepend: "$", append: ".00" } | ||
| end | ||
|
|
||
| test "file field with prepend text" do | ||
| expected = <<~HTML | ||
| <div class="mb-3"> | ||
| <label class="form-label" for="user_avatar">Avatar</label> | ||
| <div class="input-group"> | ||
| <span class="input-group-text">before</span> | ||
| <input class="form-control" id="user_avatar" name="user[avatar]" type="file" /> | ||
| </div> | ||
| </div> | ||
| HTML | ||
| assert_equivalent_html expected, @builder.file_field(:avatar, prepend: "before") | ||
| end | ||
|
|
||
| test "file field with append text" do | ||
| expected = <<~HTML | ||
| <div class="mb-3"> | ||
| <label class="form-label" for="user_avatar">Avatar</label> | ||
| <div class="input-group"> | ||
| <input class="form-control" id="user_avatar" name="user[avatar]" type="file" /> | ||
| <span class="input-group-text">after</span> | ||
| </div> | ||
| </div> | ||
| HTML | ||
| assert_equivalent_html expected, @builder.file_field(:avatar, append: "after") | ||
| end | ||
|
|
||
| test "file field with append and prepend button" do | ||
| prefix = '<div class="mb-3"><label class="form-label" for="user_avatar">Avatar</label><div class="input-group">' | ||
| field = <<~HTML | ||
| <input class="form-control" id="user_avatar" name="user[avatar]" type="file" /> | ||
| HTML | ||
| button_src = link_to("Click", "#", class: "btn btn-secondary") | ||
| button_prepend = button_src | ||
| button_append = button_src | ||
| suffix = "</div></div>" | ||
| after_button = prefix + field + button_append + suffix | ||
| before_button = prefix + button_prepend + field + suffix | ||
| both_button = prefix + button_prepend + field + button_append + suffix | ||
| multiple_button = prefix + button_prepend + button_prepend + field + button_append + button_append + suffix | ||
| assert_equivalent_html after_button, @builder.file_field(:avatar, append: button_src) | ||
| assert_equivalent_html before_button, @builder.file_field(:avatar, prepend: button_src) | ||
| assert_equivalent_html both_button, @builder.file_field(:avatar, append: button_src, prepend: button_src) | ||
| assert_equivalent_html multiple_button, @builder.file_field(:avatar, append: [button_src] * 2, prepend: [button_src] * 2) | ||
|
Comment on lines
+237
to
+240
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice set of tests!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shamelessly duped from a few lines up in the same file! 😁 |
||
| end | ||
|
|
||
| test "help messages for default forms" do | ||
| expected = <<~HTML | ||
| <div class="mb-3"> | ||
|
|
||
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.
Did you find you needed to change this? There was some dependency issue for Rails 7.1 and earlier where we have to keep SQLite back at 1.4, but if it caused you problems, leave it in. If it didn't actually break anything, let's leave it the way it was, and we'll take it out when we drop support for Rails 7.1.
Uh oh!
There was an error while loading. Please reload this page.
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.
I did, but possibly just because I am unsure how to route around this problem in my local environment. I kept getting the "you have already activated sqlite [much higher version]" when I ran the tests, both with and without bundle exec. I can take it back out and just not run the tests here... (edit: To be clear, the tests just would not run at all if I left the gemfile alone.)
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.
I have both 7.1.x and 8.0.x (rails) installed locally, under ruby 3.2.8.
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.
Thanks for the reminder. That was it. Something else already activated a later version. But for some reason we wanted to keep earlier versions of Rails on 1.4. Let's go with what you have and then I'll fix this when we drop Rails 7.1.