panel macro ignores markup_contents if table_for presents

[romul]

For example:

      panel "Children" do
        table_for(category.children) do
          column :name
        end

        markup_contents do
          h3 "Test"
        end
      end

will output only the table. Expected behavior - output both: the table and the markup.

[romul]

That's because of do_panel implementation: https://github.com/smpallen99/ex_admin/blob/master/lib/ex_admin/table.ex#L52

[jwarlander]

Yep, figured some recursion could fix that; after processing :table_for, remove that key from the map and call do_panel with remaining parts.

[jwarlander]

About to do a test on my existing app :)

[smpallen99]

@jwarlander would be great if u could add a test for this fix. Thanks... Steve

[smpallen99]

@jwarlander I would think your recursive approach might work. I'm just wondering what would/should happen if they have more than one table_for or markup_contents. Probably best if we make sure all are rendered, or we raise an appropriate exception.

[jwarlander]

My initial implementation just recurses until no remaining match, concatenating the results along the way; the default catch-all clause simply returns the result.

Will definitely add test(s) for it :-)

Right now, though, gotta get dinner ready before our daughter falls asleep ;-)

Den lör 14 maj 2016 18:25Steve Pallen notifications@github.com skrev:

@jwarlander https://github.com/jwarlander I would think your recursive approach might work. I'm just wondering what would/should happen if they have more than one table_for or markup_contents. Probably best if we make sure all are rendered, or we raise an appropriate exception.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/smpallen99/ex_admin/issues/112#issuecomment-219229361

[romul]

@smpallen99 I think, the best option would be render all blocks specified inside panel macro regardless of their quantity.

[romul]

I've added a quick and dirty fix to don't block associations stuff. But I'd like to end up with proper solution, that handle more than one table_for or markup_contents in any order.

[jwarlander]

Once I realized the HTML being built was kept in ETS, my recursion approach worked out well -- but the way the schema is passed (as a map, with :table_for or :contents as the key) doesn't allow multiple instances of each of those, just having one or both of them in either order..

  def do_panel(conn, %{table_for: %{resources: resources, columns: columns, opts: opts}} = schema, table_opts) do
    table(Dict.merge(table_opts, opts)) do
      # ...
    end
    new_schema = Map.delete(schema, :table_for)
    do_panel(conn, new_schema, table_opts)
  end
  def do_panel(conn, %{contents: %{contents: content}} = schema, table_opts) do
    div do
      content |> elem(1) |> Xain.text
    end
    new_schema = Map.delete(schema, :contents)
    do_panel(conn, new_schema, table_opts)
  end
  def do_panel(_conn, _schema, _table_opts) do
    ""
  end

If it were a keyword-style list instead, it could have multiples, and one could easily process it from beginning to end:

  def do_panel(conn, [{:table_for, %{resources: resources, columns: columns, opts: opts}}|tail], table_opts) do
    table(Dict.merge(table_opts, opts)) do
      # ...
    end
    do_panel(conn, tail, table_opts)
  end
  def do_panel(conn, [{:contents, %{contents: content}}|tail], table_opts) do
    div do
      content |> elem(1) |> Xain.text
    end
    do_panel(conn, tail, table_opts)
  end
  def do_panel(_conn, _schema, _table_opts) do
    ""
  end

[jwarlander]

Alternatively, the map key could be a string instead, and have some kind of unique identifier appended.. then you could match it like:

  def do_panel(conn, %{"table_for_" <> id, %{ ... }}, table_opts) do
    # ...
    new_schema = Map.delete(schema, "table_for_" <> id)
    do_panel(conn, new_schema, table_opts)
  end

It's not as elegant, but may be easier to implement depending on how the schema generation is organized (haven't looked yet).

[romul]

I like a keyword list approach. This data structure fits perfect for this case.

[jwarlander]

I've got it working with keyword list.. will open a pull request, just going to sync against your latest changes :)

[jwarlander]

Oh, right.. you're on a separate branch; which one should I do a pull request against?

[romul]

@jwarlander Give me a link to your branch. I'll cherry-pick commits from it to my branch and later (if all is ok) @smpallen99 will merge them all to ex_admin main repo.

[jwarlander]

You'll find it here:

https://github.com/jwarlander/ex_admin/tree/handle-multiple-panel-sections

It's just one commit - I haven't written any tests for it yet, as I just got it working. I should be able to get to that tomorrow though, once I figure out a good way to test. I'm hoping there will be some existing tests I can look at for inspiration ;)

[romul]

Great! I like your commit. I'll merge it to my branch tomorrow.

[smpallen99]

@jwrlander. I think you can add a test or two in controller test. Simply add to one of the resources in test/support/admin_resources.exs. Should be pretty straight forward.

[jwarlander]

Done; pushed the test to the branch referred above :)

Parsing test failures when looking at HTML output would be a bit nicer with something like Floki, where you can pick out a section of the raw HTML and make a nested tuple / list data structure of it; but I'm not sure if it's worth adding a dependency for.

Thanks for the pointer, @smpallen99 - wasn't hard at all!

[smpallen99]

@jwarlander Looks good. I like your idea of using Floki. I've never used that package before. Would be a good addition. I'll look at on the next test I write that would benefit. Unless someone else beats me to it :).

I'll make sure all your stuff gets pulled in with @romul pull request which i'll be reviewing tonight.

Steve