Skip to content
This repository has been archived by the owner on May 16, 2024. It is now read-only.

Table #47

Merged
merged 12 commits into from
Apr 16, 2020
Merged

Table #47

merged 12 commits into from
Apr 16, 2020

Conversation

bryanjos
Copy link
Contributor

@bryanjos bryanjos commented Apr 4, 2019

connects to #19

Table

          <%= table do %>
            <%= table_head do %>
              <%= table_row do %>
                <%= table_header do: "ID" %>
                <%= table_header do: "First Name" %>
                <%= table_header do: "Last Name" %>
              <% end %>
            <% end %>
            <%= table_body do %>
              <%= table_row do %>
                <%= table_data do: "10" %>
                <%= table_data do: "Jane" %>
                <%= table_data do: "Doe" %>
              <% end %>
            <% end %>
          <% end %>

Pagination

<%= pagination(@conn, Routes.page_path(@conn, :index), 5, 10) %>

@coveralls
Copy link

coveralls commented Apr 4, 2019

Pull Request Test Coverage Report for Build 145

  • 1 of 15 (6.67%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-6.0%) to 52.555%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/harmonium.ex 1 7 14.29%
lib/harmonium/table.ex 0 8 0.0%
Totals Coverage Status
Change from base Build 118: -6.0%
Covered Lines: 72
Relevant Lines: 137

💛 - Coveralls

@bryanjos bryanjos changed the title Add table component Add table and pagination components Apr 4, 2019
Copy link
Contributor

@brianberlin brianberlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your code is so pretty!

Copy link
Contributor

@jwietelmann jwietelmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you split this up? I have zero objections to the table stuff, but the pagination things feel rather limited in their flexibility, and I'd like to address that separately.

@bryanjos bryanjos changed the title Add table and pagination components Table Apr 4, 2019
block
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have said this before, but I think all these helper functions should follow the current standard Harmonium pattern where you can optionally pass CSS class modifiers as the 2nd arg:

  @doc """
  Constructs a callout class.
  """
  def callout_class(modifiers \\ []), do: rev_class(@callout_class, modifiers)

  @doc """
  Renders a callout.
  """
  def callout(do: block), do: callout([], do: block)

  def callout(modifiers, do: block), do: callout(:div, modifiers, do: block)

  def callout(tag, modifiers, do: block) do
    content_tag tag, class: callout_class(modifiers) do
      block
    end
  end

Our Harmonium SCSS Table does include some very useful SUIT modifiers: https://github.com/revelrylabs/harmonium/blob/master/scss/components/_Table.scss

import Phoenix.HTML.Tag

def table(opts \\ [], do: block) do
table_element(:table, [class: "rev-Table"], opts, do: block)
Copy link
Contributor

@brianberlin brianberlin Apr 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change all of these to...

def table_data(opts \\ []) do
    {block, opts} = Keyword.pop(opts, :do, nil)
    table_element(:td, [class: "rev-Table"], opts, do: block)
end

Then we can do something like...

<%= table_data colspan: 5 %>
<%= table_data do: sum %>

otherwise you have to do

<%= table_data colspan: 5 do %>
  <%= sum %>
<% end %>

or you can also do this i guess but be nice to the other way i think

table_data([colspan: 5], [do: nil])

@brianberlin
Copy link
Contributor

Should table add the .rev-TableContainer wrapper?

@brianberlin brianberlin merged commit 41aa64f into master Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants