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

Nested Form Fields Rendered Twice in 1.2.0 Release #119

Closed
macfanatic opened this issue Apr 8, 2019 · 11 comments · Fixed by #121
Closed

Nested Form Fields Rendered Twice in 1.2.0 Release #119

macfanatic opened this issue Apr 8, 2019 · 11 comments · Fixed by #121

Comments

@macfanatic
Copy link

macfanatic commented Apr 8, 2019

After upgrading to the latest activeadmin and arbre, we realized that our nested form fields are being duplicated in our templates.

  • activeadmin 1.3.1
  • arbre 1.2.0
  • rails 5.2.3
  • ruby 2.3.6

Example form for ActiveAdmin resource:

ActiveAdmin.register Drawer do
  form do |f|
    columns do
      column do
        panel 'Payment Methods' do
          table do
            thead do
              th 'Payment Method'
              th 'Amount Processed'
              th 'Expected on Hand'
            end

            tbody do
              f.semantic_fields_for :payments do |payments|
                tr(class: cycle('odd', '')) do
                  td(class: 'hidden') { payments.input :id, as: :hidden }
                  td { payments.object.payment_method.name }
                  td { payments.object.amount_processed.format }
                  td { payments.object.amount_expected_on_hand.format }
                end
              end
            end
          end
        end
      end
    end
  end
end

We end up with the semantic_fields_for rendering double the amount of rows, a whole group and then the entire collection again a second time.

Leaving activeadmin and rails locked to the versions mentioned above, but downgrading to arbre version 1.1.1 (the previous release) fixes the issue.

@deivid-rodriguez
Copy link
Member

Thansk for the report, I'll have a look 👍

@timoschilling
Copy link
Member

How does it look with arbre 1.1.1?

@macfanatic
Copy link
Author

macfanatic commented Apr 9, 2019

Apologies, typo - we are locked and using arbre 1.1.1 as the fix, not 1.1.0 as written.

I updated the issue info above.

@deivid-rodriguez
Copy link
Member

What about activeadmin 2.0.0.rc1? Does it work there? Also, can you provide a sample application reproducing the regression?

@macfanatic
Copy link
Author

The issue exists in all the following configurations:

  • arbre 1.2.0.rc1 and activeadmin 1.3.1
  • arbre 1.2.0 and activeadmin 1.3.1
  • arbre 1.2.0 and activeadmin 2.0.0.rc1

The issue does not exist with:

  • arbre 1.1.1 and activeadmin 1.3.1
  • arbre 1.1.1 and activeadmin 2.0.0.rc

It very much appears to have been introduced and present in both the arbre RC1 and final versions.

I'll see if I can get a demo repo setup and shared to reproduce.

@macfanatic
Copy link
Author

Here is a link to a reproduction repo, the README includes setup and what to do for viewing the issue.

https://github.com/madebylotus/arbre-regression-1.2.0-example

@deivid-rodriguez
Copy link
Member

Nice, thanks so much @macfanatic!

@deivid-rodriguez
Copy link
Member

Run a bisection over you app and the culprit seems #64.

@seanlinsley
Copy link
Contributor

Here's another example where double rendering happens: one form accidentally included f.actions inside of f.inputs, leading to the submit and cancel buttons to be double-rendered. It was easy enough to fix by moving it out of the inputs block.

  form do |f|
    f.inputs do
      # ...

      f.actions
    end
  end

Screen Shot 2019-04-11 at 10 14 32 AM

@deivid-rodriguez
Copy link
Member

I worked a bit on this today in this branch.

It sounds like regardless of how we end up fixing #46, we should probably revert #64.

@varyonic
Copy link
Contributor

madebylotus/arbre-regression-1.2.0-example#1

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 a pull request may close this issue.

5 participants