From ebca4d41e942ecf81a401ef0cfe27c9d79613fc7 Mon Sep 17 00:00:00 2001 From: Peter Yates Date: Sat, 24 Dec 2022 15:39:15 +0000 Subject: [PATCH 1/7] Convert head, body and row slots to lambdas As a precursor to supporting scopes this will give us a better way of applying default values. Currently when a cell is rendered we don't know if it's in the head or body of a table so can't toggle the default scope automatically[0]. [0] https://design-system.service.gov.uk/components/table/#table-headers --- .../table_component/body_component.rb | 10 +++++++++- .../table_component/head_component.rb | 10 +++++++++- .../table_component/row_component.rb | 13 ++++++++++++- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/app/components/govuk_component/table_component/body_component.rb b/app/components/govuk_component/table_component/body_component.rb index a8368636..89258051 100644 --- a/app/components/govuk_component/table_component/body_component.rb +++ b/app/components/govuk_component/table_component/body_component.rb @@ -1,5 +1,13 @@ class GovukComponent::TableComponent::BodyComponent < GovukComponent::Base - renders_many :rows, "GovukComponent::TableComponent::RowComponent" + renders_many :rows, ->(cell_data: nil, first_cell_is_header: false, classes: [], html_attributes: {}, &block) do + GovukComponent::TableComponent::RowComponent.new( + cell_data: cell_data, + first_cell_is_header: first_cell_is_header, + classes: classes, + html_attributes: html_attributes, + &block + ) + end def initialize(rows: nil, first_cell_is_header: false, classes: [], html_attributes: {}) super(classes: classes, html_attributes: html_attributes) diff --git a/app/components/govuk_component/table_component/head_component.rb b/app/components/govuk_component/table_component/head_component.rb index 8863f60e..96d964ba 100644 --- a/app/components/govuk_component/table_component/head_component.rb +++ b/app/components/govuk_component/table_component/head_component.rb @@ -1,5 +1,13 @@ class GovukComponent::TableComponent::HeadComponent < GovukComponent::Base - renders_many :rows, "GovukComponent::TableComponent::RowComponent" + renders_many :rows, ->(cell_data: nil, header: true, classes: [], html_attributes: {}, &block) do + GovukComponent::TableComponent::RowComponent.new( + cell_data: cell_data, + header: header, + classes: classes, + html_attributes: html_attributes, + &block + ) + end attr_reader :row_data diff --git a/app/components/govuk_component/table_component/row_component.rb b/app/components/govuk_component/table_component/row_component.rb index 2e827604..528a8e77 100644 --- a/app/components/govuk_component/table_component/row_component.rb +++ b/app/components/govuk_component/table_component/row_component.rb @@ -1,5 +1,16 @@ class GovukComponent::TableComponent::RowComponent < GovukComponent::Base - renders_many :cells, "GovukComponent::TableComponent::CellComponent" + renders_many :cells, ->(header: false, scope: nil, text: nil, numeric: false, width: nil, classes: [], html_attributes: {}, &block) do + GovukComponent::TableComponent::CellComponent.new( + header: header, + text: text, + numeric: numeric, + width: width, + scope: scope || cell_scope(header, parent), + classes: classes, + html_attributes: html_attributes, + &block + ) + end attr_reader :header, :first_cell_is_header From 39d9b8103cdc90f4886fbd71d5e51e20b52e6f08 Mon Sep 17 00:00:00 2001 From: Peter Yates Date: Sat, 24 Dec 2022 19:10:06 +0000 Subject: [PATCH 2/7] Add scopes attr and default values to table header Table header cells `th` can appear in two places: * Usually they're only present in the `thead` as column headers so they have the attribute `scope='col'`. * Sometimes they are also used in the first column to mark the row header, then they have the attribute `scope='row'`. This commit adds a 'scope' parameter to the CellComponent. It's a required parameter but it's automatically set in the `#cell_scope` method at the row level if it's missing, so unless you're using the CellComponent standalone you should never have to set it. To determine the kind of scope we also need to know whether the grandparent of the cell is the `thead` or `tbody`, so `parent` has been added as a parameter to the RowComponent. Again, it will need to be manually set if the component is used standalone but when used within a table it's automatically assigned. --- .../table_component/body_component.rb | 3 +- .../table_component/cell_component.rb | 12 ++++-- .../table_component/head_component.rb | 3 +- .../table_component/row_component.rb | 26 +++++++++++-- .../govuk_component/table_component_spec.rb | 37 +++++++++++++++---- 5 files changed, 65 insertions(+), 16 deletions(-) diff --git a/app/components/govuk_component/table_component/body_component.rb b/app/components/govuk_component/table_component/body_component.rb index 89258051..e5a783ef 100644 --- a/app/components/govuk_component/table_component/body_component.rb +++ b/app/components/govuk_component/table_component/body_component.rb @@ -1,8 +1,9 @@ class GovukComponent::TableComponent::BodyComponent < GovukComponent::Base - renders_many :rows, ->(cell_data: nil, first_cell_is_header: false, classes: [], html_attributes: {}, &block) do + renders_many :rows, ->(cell_data: nil, first_cell_is_header: false, parent: 'tbody', classes: [], html_attributes: {}, &block) do GovukComponent::TableComponent::RowComponent.new( cell_data: cell_data, first_cell_is_header: first_cell_is_header, + parent: parent, classes: classes, html_attributes: html_attributes, &block diff --git a/app/components/govuk_component/table_component/cell_component.rb b/app/components/govuk_component/table_component/cell_component.rb index 1f880d42..2d121c1c 100644 --- a/app/components/govuk_component/table_component/cell_component.rb +++ b/app/components/govuk_component/table_component/cell_component.rb @@ -1,5 +1,5 @@ class GovukComponent::TableComponent::CellComponent < GovukComponent::Base - attr_reader :text, :header, :numeric, :width + attr_reader :text, :header, :numeric, :width, :scope alias_method :numeric?, :numeric @@ -12,11 +12,12 @@ class GovukComponent::TableComponent::CellComponent < GovukComponent::Base "one-quarter" => "govuk-!-width-one-quarter", }.freeze - def initialize(header: false, text: nil, numeric: false, width: nil, classes: [], html_attributes: {}) + def initialize(header: false, scope:, text: nil, numeric: false, width: nil, classes: [], html_attributes: {}) @header = header @text = text @numeric = numeric @width = width + @scope = scope super(classes: classes, html_attributes: html_attributes) end @@ -36,11 +37,14 @@ def cell_content end def cell_element - header ? :th : :td + header ? 'th' : 'td' end def default_attributes - { class: default_classes } + { + class: default_classes, + scope: scope + } end def default_classes diff --git a/app/components/govuk_component/table_component/head_component.rb b/app/components/govuk_component/table_component/head_component.rb index 96d964ba..313bf8c3 100644 --- a/app/components/govuk_component/table_component/head_component.rb +++ b/app/components/govuk_component/table_component/head_component.rb @@ -1,8 +1,9 @@ class GovukComponent::TableComponent::HeadComponent < GovukComponent::Base - renders_many :rows, ->(cell_data: nil, header: true, classes: [], html_attributes: {}, &block) do + renders_many :rows, ->(cell_data: nil, header: true, parent: 'thead', classes: [], html_attributes: {}, &block) do GovukComponent::TableComponent::RowComponent.new( cell_data: cell_data, header: header, + parent: parent, classes: classes, html_attributes: html_attributes, &block diff --git a/app/components/govuk_component/table_component/row_component.rb b/app/components/govuk_component/table_component/row_component.rb index 528a8e77..c1045515 100644 --- a/app/components/govuk_component/table_component/row_component.rb +++ b/app/components/govuk_component/table_component/row_component.rb @@ -12,11 +12,12 @@ class GovukComponent::TableComponent::RowComponent < GovukComponent::Base ) end - attr_reader :header, :first_cell_is_header + attr_reader :header, :first_cell_is_header, :parent - def initialize(cell_data: nil, first_cell_is_header: false, header: false, classes: [], html_attributes: {}) + def initialize(parent:, cell_data: nil, first_cell_is_header: false, header: false, classes: [], html_attributes: {}) @header = header @first_cell_is_header = first_cell_is_header + @parent = parent if parent_valid?(parent) super(classes: classes, html_attributes: html_attributes) @@ -25,10 +26,29 @@ def initialize(cell_data: nil, first_cell_is_header: false, header: false, class private + def parent_valid?(supplied_parent) + return true if supplied_parent.nil? + return true if supplied_parent.in?(%w(thead tbody)) + + fail(ArgumentError, "invalid parent value #{parent}, must be either 'thead' or 'tbody'") + end + def build_cells_from_cell_data(cell_data) return if cell_data.blank? - cell_data.map.with_index { |cd, i| cell(header: cell_is_header?(i), text: cd) } + cell_data.each.with_index { |data, i| cell(text: data, **cell_attributes(i)) } + end + + def cell_attributes(count) + cell_is_header?(count).then do |cell_is_header| + { header: cell_is_header, scope: cell_scope(cell_is_header, parent) } + end + end + + def cell_scope(is_header, parent) + return unless is_header + + parent == 'thead' ? 'col' : 'row' end def cell_is_header?(count) diff --git a/spec/components/govuk_component/table_component_spec.rb b/spec/components/govuk_component/table_component_spec.rb index e95fed5d..294f8f4a 100644 --- a/spec/components/govuk_component/table_component_spec.rb +++ b/spec/components/govuk_component/table_component_spec.rb @@ -141,7 +141,9 @@ specify "renders one header row" do expect(rendered_content).to have_tag("table", with: { class: component_css_class }) do with_tag("thead", with: { class: "govuk-table__head" }) do - with_tag("tr", with: { class: "govuk-table__row" }, count: 1) + with_tag("tr", with: { class: "govuk-table__row" }, count: 1) do + with_tag("th", with: { class: "govuk-table__header", scope: "col" }, count: 4) + end end end end @@ -193,9 +195,17 @@ render_inline(GovukComponent::TableComponent.new(**kwargs.merge(head: head, rows: rows, first_cell_is_header: true))) end - specify "renders the header content in table header (th) cells" do + specify "renders the table header" do + expect(rendered_content).to have_tag("table > thead") do + head.each do |heading| + with_tag('th', text: heading, with: { class: 'govuk-table__header', scope: 'col' }) + end + end + end + + specify "renders the header column in table body cells" do expect(rendered_content).to have_tag("table > tbody") do - with_tag('th', text: row_header_text, count: number_of_rows) + with_tag('th', text: row_header_text, count: number_of_rows, with: { scope: 'row' }) with_tag('td', text: row_cell_text, count: number_of_rows * 2) end end @@ -203,6 +213,15 @@ specify "the header is always first" do html.css('tbody > tr').map(&:elements).each { |r| expect(r.first.name).to eql('th') } end + + specify "all header cells in the tbody have a scope but no data cells do" do + attributes_per_element_type = %w(th td).each.with_object({}) do |element, h| + h[element] = html.css(element).map(&:attributes).map(&:keys) + end + + expect(attributes_per_element_type["th"]).to all(eql(%w(class scope))) + expect(attributes_per_element_type["td"]).to all(eql(%w(class))) + end end end @@ -220,7 +239,7 @@ body.row do |row| row.cell(text: "row-#{i}-col-1") row.cell(text: "row-#{i}-col-2") - row.cell(text: "row-#{i}-col-3") + row.cell(text: "row-#{i}-col-3", scope: "bananas") end end end @@ -235,7 +254,7 @@ expect(rendered_content).to have_tag('table') do with_tag('thead') do with_tag('tr', with: { class: "govuk-table__row" }, count: 1) do - with_tag('th', with: { class: "govuk-table__header" }, count: 3) + with_tag('th', with: { class: "govuk-table__header", scope: "col" }, count: 3) end end end @@ -252,6 +271,10 @@ end end end + + specify "scopes can be set to arbitrary values" do + expect(rendered_content).to have_tag("td", with: { scope: "bananas" }, count: 3) + end end context 'when some data is numeric' do @@ -394,7 +417,7 @@ RSpec.describe(GovukComponent::TableComponent::RowComponent, type: :component) do let(:component_css_class) { 'govuk-table__row' } - let(:kwargs) { {} } + let(:kwargs) { { parent: 'tbody' } } it_behaves_like 'a component that accepts custom classes' it_behaves_like 'a component that accepts custom HTML attributes' @@ -402,7 +425,7 @@ RSpec.describe(GovukComponent::TableComponent::CellComponent, type: :component) do let(:component_css_class) { 'govuk-table__cell' } - let(:kwargs) { {} } + let(:kwargs) { { scope: nil } } it_behaves_like 'a component that accepts custom classes' it_behaves_like 'a component that accepts custom HTML attributes' From 8cfdd5f44a0ad5c21644645e178f64c60e4baa36 Mon Sep 17 00:00:00 2001 From: Peter Yates Date: Mon, 26 Dec 2022 19:30:16 +0000 Subject: [PATCH 3/7] Add a 'first_cell_is_header' example to the guide --- .../table_component/cell_component.rb | 2 +- guide/content/components/table.slim | 3 +++ guide/lib/examples/table_helpers.rb | 12 ++++++------ 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/app/components/govuk_component/table_component/cell_component.rb b/app/components/govuk_component/table_component/cell_component.rb index 2d121c1c..a13ea644 100644 --- a/app/components/govuk_component/table_component/cell_component.rb +++ b/app/components/govuk_component/table_component/cell_component.rb @@ -12,7 +12,7 @@ class GovukComponent::TableComponent::CellComponent < GovukComponent::Base "one-quarter" => "govuk-!-width-one-quarter", }.freeze - def initialize(header: false, scope:, text: nil, numeric: false, width: nil, classes: [], html_attributes: {}) + def initialize(scope:, header: false, numeric: false, text: nil, width: nil, classes: [], html_attributes: {}) @header = header @text = text @numeric = numeric diff --git a/guide/content/components/table.slim b/guide/content/components/table.slim index f36d54ac..35aa596d 100644 --- a/guide/content/components/table.slim +++ b/guide/content/components/table.slim @@ -47,6 +47,9 @@ p Use the table component to make information easier to compare and scan for using the `head` argument. If nothing is set, the first row will be used for headers. + The `first_cell_is_header` parameter can be used to change the first column + in the table body to header cells with `scope='row'`. + == render('/partials/example.*', caption: "Table with resized columns", code: table_with_resized_columns) do diff --git a/guide/lib/examples/table_helpers.rb b/guide/lib/examples/table_helpers.rb index 06921b86..a5177cf2 100644 --- a/guide/lib/examples/table_helpers.rb +++ b/guide/lib/examples/table_helpers.rb @@ -60,7 +60,7 @@ def table_with_header_column def table_from_arrays <<~TABLE - = govuk_table(rows: data, caption: "Pokémon species and types") + = govuk_table(rows: data, caption: "Pokémon species and types", first_cell_is_header: true) TABLE end @@ -68,11 +68,11 @@ def table_data <<~TABLE_DATA { data: [ - ["Name", "Primary type"], - ["Weedle", "Bug"], - ["Rattata", "Normal"], - ["Raichu", "Electric"], - ["Golduck", "Water"] + ["Name" , "Primary type", "Catch rate", "Other types"], + ["Weedle" , "Bug" , 255 , "Poison"], + ["Rattata", "Normal" , 255 , "Dark"], + ["Raichu" , "Electric" , 75 , "Psychic"], + ["Golduck", "Water" , 75 , "No other types"] ] } TABLE_DATA From 04260fbb062b19709b810da93cdaf280dfa6c499 Mon Sep 17 00:00:00 2001 From: Peter Yates Date: Thu, 29 Dec 2022 14:41:01 +0000 Subject: [PATCH 4/7] Reorganise and move logic to the cell Trying to make the row responsible for the cell's default scope was problematic because should we want to override it at the cell level we don't know whether it was set manually or defaulted. I originally tried this by passing another param but it's cleaner to just move the logic to the cell and pass the 'parent' down. Not sure parent is the best name for it, perhaps 'region'. At the second level it's actually a grandparent. --- .../table_component/body_component.rb | 5 ++-- .../table_component/cell_component.rb | 25 +++++++++++----- .../table_component/head_component.rb | 5 ++-- .../table_component/row_component.rb | 30 ++++++++----------- .../govuk_component/table_component_spec.rb | 7 +++-- 5 files changed, 39 insertions(+), 33 deletions(-) diff --git a/app/components/govuk_component/table_component/body_component.rb b/app/components/govuk_component/table_component/body_component.rb index e5a783ef..4f032373 100644 --- a/app/components/govuk_component/table_component/body_component.rb +++ b/app/components/govuk_component/table_component/body_component.rb @@ -1,9 +1,8 @@ class GovukComponent::TableComponent::BodyComponent < GovukComponent::Base - renders_many :rows, ->(cell_data: nil, first_cell_is_header: false, parent: 'tbody', classes: [], html_attributes: {}, &block) do - GovukComponent::TableComponent::RowComponent.new( + renders_many :rows, ->(cell_data: nil, first_cell_is_header: false, classes: [], html_attributes: {}, &block) do + GovukComponent::TableComponent::RowComponent.from_body( cell_data: cell_data, first_cell_is_header: first_cell_is_header, - parent: parent, classes: classes, html_attributes: html_attributes, &block diff --git a/app/components/govuk_component/table_component/cell_component.rb b/app/components/govuk_component/table_component/cell_component.rb index a13ea644..6a04765f 100644 --- a/app/components/govuk_component/table_component/cell_component.rb +++ b/app/components/govuk_component/table_component/cell_component.rb @@ -1,7 +1,8 @@ class GovukComponent::TableComponent::CellComponent < GovukComponent::Base - attr_reader :text, :header, :numeric, :width, :scope + attr_reader :text, :header, :numeric, :width, :scope, :parent alias_method :numeric?, :numeric + alias_method :header?, :header WIDTHS = { "full" => "govuk-!-width-full", @@ -12,12 +13,13 @@ class GovukComponent::TableComponent::CellComponent < GovukComponent::Base "one-quarter" => "govuk-!-width-one-quarter", }.freeze - def initialize(scope:, header: false, numeric: false, text: nil, width: nil, classes: [], html_attributes: {}) + def initialize(scope: nil, header: false, numeric: false, text: nil, width: nil, parent: nil, classes: [], html_attributes: {}) @header = header @text = text @numeric = numeric @width = width @scope = scope + @parent = parent super(classes: classes, html_attributes: html_attributes) end @@ -41,17 +43,24 @@ def cell_element end def default_attributes - { - class: default_classes, - scope: scope - } + { class: default_classes, scope: (scope || default_scope) } + end + + def default_scope + return unless header? + return 'col' if parent == 'thead' + return 'row' if parent == 'tbody' + + # FIXME: tfoot? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/tfoot + + fail(ArgumentError, "invalid parent") end def default_classes if header - class_names("govuk-table__header", "govuk-table__header--numeric" => numeric?, width_class => width?).split + class_names("govuk-table__header", "govuk-table__header--numeric" => numeric?, width_class => width?) else - class_names("govuk-table__cell", "govuk-table__cell--numeric" => numeric?, width_class => width?).split + class_names("govuk-table__cell", "govuk-table__cell--numeric" => numeric?, width_class => width?) end end diff --git a/app/components/govuk_component/table_component/head_component.rb b/app/components/govuk_component/table_component/head_component.rb index 313bf8c3..90a3e021 100644 --- a/app/components/govuk_component/table_component/head_component.rb +++ b/app/components/govuk_component/table_component/head_component.rb @@ -1,9 +1,8 @@ class GovukComponent::TableComponent::HeadComponent < GovukComponent::Base - renders_many :rows, ->(cell_data: nil, header: true, parent: 'thead', classes: [], html_attributes: {}, &block) do - GovukComponent::TableComponent::RowComponent.new( + renders_many :rows, ->(cell_data: nil, header: true, classes: [], html_attributes: {}, &block) do + GovukComponent::TableComponent::RowComponent.from_head( cell_data: cell_data, header: header, - parent: parent, classes: classes, html_attributes: html_attributes, &block diff --git a/app/components/govuk_component/table_component/row_component.rb b/app/components/govuk_component/table_component/row_component.rb index c1045515..cb28d444 100644 --- a/app/components/govuk_component/table_component/row_component.rb +++ b/app/components/govuk_component/table_component/row_component.rb @@ -1,11 +1,12 @@ class GovukComponent::TableComponent::RowComponent < GovukComponent::Base - renders_many :cells, ->(header: false, scope: nil, text: nil, numeric: false, width: nil, classes: [], html_attributes: {}, &block) do + renders_many :cells, ->(scope: nil, header: false, text: nil, numeric: false, width: nil, classes: [], html_attributes: {}, &block) do GovukComponent::TableComponent::CellComponent.new( + scope: scope, header: header, text: text, numeric: numeric, width: width, - scope: scope || cell_scope(header, parent), + parent: parent, classes: classes, html_attributes: html_attributes, &block @@ -14,25 +15,26 @@ class GovukComponent::TableComponent::RowComponent < GovukComponent::Base attr_reader :header, :first_cell_is_header, :parent - def initialize(parent:, cell_data: nil, first_cell_is_header: false, header: false, classes: [], html_attributes: {}) + def initialize(cell_data: nil, first_cell_is_header: false, header: false, parent: nil, classes: [], html_attributes: {}) @header = header @first_cell_is_header = first_cell_is_header - @parent = parent if parent_valid?(parent) + @parent = parent super(classes: classes, html_attributes: html_attributes) build_cells_from_cell_data(cell_data) end -private - - def parent_valid?(supplied_parent) - return true if supplied_parent.nil? - return true if supplied_parent.in?(%w(thead tbody)) + def self.from_head(*args, **kwargs, &block) + new(*args, parent: 'thead', **kwargs, &block) + end - fail(ArgumentError, "invalid parent value #{parent}, must be either 'thead' or 'tbody'") + def self.from_body(*args, **kwargs, &block) + new(*args, parent: 'tbody', **kwargs, &block) end +private + def build_cells_from_cell_data(cell_data) return if cell_data.blank? @@ -41,16 +43,10 @@ def build_cells_from_cell_data(cell_data) def cell_attributes(count) cell_is_header?(count).then do |cell_is_header| - { header: cell_is_header, scope: cell_scope(cell_is_header, parent) } + { header: cell_is_header } end end - def cell_scope(is_header, parent) - return unless is_header - - parent == 'thead' ? 'col' : 'row' - end - def cell_is_header?(count) header || (first_cell_is_header && count.zero?) end diff --git a/spec/components/govuk_component/table_component_spec.rb b/spec/components/govuk_component/table_component_spec.rb index 294f8f4a..baa3aa85 100644 --- a/spec/components/govuk_component/table_component_spec.rb +++ b/spec/components/govuk_component/table_component_spec.rb @@ -22,7 +22,10 @@ end specify "renders a table with thead and tbody elements" do - expect(rendered_content).to have_tag("table", with: { class: "govuk-table" }) + expect(rendered_content).to have_tag("table", with: { class: "govuk-table" }) do + with_tag('thead', with: { class: "govuk-table__head" }) + with_tag('tbody', with: { class: "govuk-table__body" }) + end end specify "table has the provided id" do @@ -417,7 +420,7 @@ RSpec.describe(GovukComponent::TableComponent::RowComponent, type: :component) do let(:component_css_class) { 'govuk-table__row' } - let(:kwargs) { { parent: 'tbody' } } + let(:kwargs) { {} } it_behaves_like 'a component that accepts custom classes' it_behaves_like 'a component that accepts custom HTML attributes' From 04fddfed6a9bd68c6f05073c4e40f7fcfe0c8f86 Mon Sep 17 00:00:00 2001 From: Peter Yates Date: Thu, 29 Dec 2022 15:32:05 +0000 Subject: [PATCH 5/7] Reorganise scope determination logic, add specs The specs cover the suppression of scopes using `scope: false` at the cell level and overriding the automatic scope logic completely by providing a hardcoded value. --- .../table_component/cell_component.rb | 21 +++++++---- .../govuk_component/table_component_spec.rb | 37 +++++++++++++++++++ 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/app/components/govuk_component/table_component/cell_component.rb b/app/components/govuk_component/table_component/cell_component.rb index 6a04765f..9e977fe6 100644 --- a/app/components/govuk_component/table_component/cell_component.rb +++ b/app/components/govuk_component/table_component/cell_component.rb @@ -43,17 +43,22 @@ def cell_element end def default_attributes - { class: default_classes, scope: (scope || default_scope) } + { class: default_classes, scope: determine_scope } end - def default_scope - return unless header? - return 'col' if parent == 'thead' - return 'row' if parent == 'tbody' + def determine_scope + conditions = { scope: scope, parent: parent, header: header } - # FIXME: tfoot? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/tfoot - - fail(ArgumentError, "invalid parent") + case conditions + in { scope: String } + scope + in { scope: false } | { header: false } + nil + in { parent: 'thead' } + 'col' + in { parent: 'tbody' } + 'row' + end end def default_classes diff --git a/spec/components/govuk_component/table_component_spec.rb b/spec/components/govuk_component/table_component_spec.rb index baa3aa85..b5530a42 100644 --- a/spec/components/govuk_component/table_component_spec.rb +++ b/spec/components/govuk_component/table_component_spec.rb @@ -432,6 +432,43 @@ it_behaves_like 'a component that accepts custom classes' it_behaves_like 'a component that accepts custom HTML attributes' + + describe "controlling scopes" do + subject! do + render_inline(GovukComponent::TableComponent::RowComponent.new(parent: 'thead')) do |row| + row.cell( + text: "ABC", + scope: false, + header: true, + html_attributes: { class: "scope_is_false" }, + ) + row.cell( + text: "DEF", + scope: true, + header: true, + html_attributes: { class: "scope_is_true" }, + ) + row.cell( + text: "GHI", + scope: "custom", + header: false, + html_attributes: { class: "scope_on_td" }, + ) + end + end + + it "suppresses the scope attribute when scope: false" do + expect(html.at_css('th.scope_is_false').attributes.keys).to match_array(%w(class)) + end + + it "doesn't suppress the scope attribute when scope: true" do + expect(html.at_css('th.scope_is_true').attributes.keys).to match_array(%w(class scope)) + end + + it "sets the custom scope when scope: 'custom'" do + expect(rendered_content).to have_tag('td', with: { class: 'scope_on_td', scope: 'custom' }) + end + end end RSpec.describe(GovukComponent::TableComponent::CaptionComponent, type: :component) do From 5d48b5a3c5677830f4869fc7cc216f6b73fe9282 Mon Sep 17 00:00:00 2001 From: Peter Yates Date: Thu, 29 Dec 2022 15:56:44 +0000 Subject: [PATCH 6/7] Make a config option for auto scope behaviour This should make it easier to introduce the feature without affecting apps. Currently it's enabled by default but we might be better starting disabled and enabling it at the next major revision. --- .../table_component/cell_component.rb | 12 ++++-- lib/govuk/components/engine.rb | 2 + .../table_component_configuration_spec.rb | 37 +++++++++++++++++++ 3 files changed, 47 insertions(+), 4 deletions(-) create mode 100644 spec/components/govuk_component/configuration/table_component_configuration_spec.rb diff --git a/app/components/govuk_component/table_component/cell_component.rb b/app/components/govuk_component/table_component/cell_component.rb index 9e977fe6..4d5cdf14 100644 --- a/app/components/govuk_component/table_component/cell_component.rb +++ b/app/components/govuk_component/table_component/cell_component.rb @@ -47,17 +47,21 @@ def default_attributes end def determine_scope - conditions = { scope: scope, parent: parent, header: header } + conditions = { scope: scope, parent: parent, header: header, auto_table_scopes: config.enable_auto_table_scopes } case conditions in { scope: String } scope - in { scope: false } | { header: false } + in { scope: false } | { header: false } | { auto_table_scopes: false } nil - in { parent: 'thead' } + in { auto_table_scopes: true, parent: 'thead' } 'col' - in { parent: 'tbody' } + in { auto_table_scopes: true, parent: 'tbody' } 'row' + else + Rails.logger.warning("No scope pattern matched") + + nil end end diff --git a/lib/govuk/components/engine.rb b/lib/govuk/components/engine.rb index 9353617c..6ea59926 100644 --- a/lib/govuk/components/engine.rb +++ b/lib/govuk/components/engine.rb @@ -64,6 +64,7 @@ def reset! # +:default_warning_text_icon+ "!" # # +:require_summary_list_action_visually_hidden_text+ when true forces visually hidden text to be set for every action. It can still be explicitly skipped by passing in +nil+. Defaults to +false+ + # +:enable_auto_table_scopes+ automatically adds a scope of 'col' to th elements in thead and 'row' to th elements in tbody. DEFAULTS = { default_back_link_text: 'Back', default_breadcrumbs_collapse_on_mobile: false, @@ -99,6 +100,7 @@ def reset! default_link_new_tab_text: "(opens in new tab)", require_summary_list_action_visually_hidden_text: false, + enable_auto_table_scopes: true, }.freeze DEFAULTS.each_key { |k| config_accessor(k) { DEFAULTS[k] } } diff --git a/spec/components/govuk_component/configuration/table_component_configuration_spec.rb b/spec/components/govuk_component/configuration/table_component_configuration_spec.rb new file mode 100644 index 00000000..3c0c1f3c --- /dev/null +++ b/spec/components/govuk_component/configuration/table_component_configuration_spec.rb @@ -0,0 +1,37 @@ +require 'spec_helper' + +RSpec.describe(GovukComponent::TableComponent::CellComponent, type: :component) do + let(:text) { 'Content' } + let(:kwargs) { { text: text, header: true, parent: 'tbody' } } + let(:component_css_class) { 'govuk-table__cell' } + + describe 'configuration' do + describe 'disabling automatic scopes' do + context "when enable_auto_table_scopes: true" do + subject! { render_inline(GovukComponent::TableComponent::CellComponent.new(**kwargs)) } + + specify "renders a scopeless table cell" do + expect(rendered_content).to have_tag("th", text: "Content") + expect(html.at_css('th').attributes.keys).to match_array(%w(class scope)) + end + end + + context "when enable_auto_table_scopes: false" do + after { Govuk::Components.reset! } + + before do + Govuk::Components.configure do |config| + config.enable_auto_table_scopes = false + end + end + + subject! { render_inline(GovukComponent::TableComponent::CellComponent.new(**kwargs)) } + + specify "renders a scopeless table cell" do + expect(rendered_content).to have_tag("th", text: "Content") + expect(html.at_css('th').attributes.keys).to match_array(%w(class)) + end + end + end + end +end From f68a2a5698f95301fd1ada347b5e36ede9c69af0 Mon Sep 17 00:00:00 2001 From: Peter Yates Date: Fri, 30 Dec 2022 21:11:07 +0000 Subject: [PATCH 7/7] Convert the table component templates to methods They're all really basic and none contain any logic, there's no real need to rely on templates. --- app/components/govuk_component/table_component.html.erb | 7 ------- app/components/govuk_component/table_component.rb | 4 ++++ .../table_component/body_component.html.erb | 5 ----- .../govuk_component/table_component/body_component.rb | 4 ++++ .../table_component/head_component.html.erb | 5 ----- .../govuk_component/table_component/head_component.rb | 4 ++++ .../govuk_component/table_component/row_component.html.erb | 5 ----- .../govuk_component/table_component/row_component.rb | 4 ++++ 8 files changed, 16 insertions(+), 22 deletions(-) delete mode 100644 app/components/govuk_component/table_component.html.erb delete mode 100644 app/components/govuk_component/table_component/body_component.html.erb delete mode 100644 app/components/govuk_component/table_component/head_component.html.erb delete mode 100644 app/components/govuk_component/table_component/row_component.html.erb diff --git a/app/components/govuk_component/table_component.html.erb b/app/components/govuk_component/table_component.html.erb deleted file mode 100644 index 81bfeda2..00000000 --- a/app/components/govuk_component/table_component.html.erb +++ /dev/null @@ -1,7 +0,0 @@ -<%= tag.table(**html_attributes) do %> - <%= caption %> - <%= head %> - <% bodies.each do |body| %> - <%= body %> - <% end %> -<% end %> diff --git a/app/components/govuk_component/table_component.rb b/app/components/govuk_component/table_component.rb index 1b0e66b6..cd1c731b 100644 --- a/app/components/govuk_component/table_component.rb +++ b/app/components/govuk_component/table_component.rb @@ -20,6 +20,10 @@ def initialize(id: nil, rows: nil, head: nil, caption: nil, first_cell_is_header build(*(head ? [head, rows] : [rows[0], rows[1..]]), caption_text) end + def call + tag.table(**html_attributes) { safe_join([caption, head, bodies]) } + end + private def build(head_data, body_data, caption_text) diff --git a/app/components/govuk_component/table_component/body_component.html.erb b/app/components/govuk_component/table_component/body_component.html.erb deleted file mode 100644 index 38e338b0..00000000 --- a/app/components/govuk_component/table_component/body_component.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -<%= tag.tbody(**html_attributes) do %> - <% rows.each do |row| %> - <%= row %> - <% end %> -<% end %> diff --git a/app/components/govuk_component/table_component/body_component.rb b/app/components/govuk_component/table_component/body_component.rb index 4f032373..b014ff66 100644 --- a/app/components/govuk_component/table_component/body_component.rb +++ b/app/components/govuk_component/table_component/body_component.rb @@ -15,6 +15,10 @@ def initialize(rows: nil, first_cell_is_header: false, classes: [], html_attribu build_rows_from_row_data(rows, first_cell_is_header) end + def call + tag.tbody(**html_attributes) { safe_join(rows) } + end + private def build_rows_from_row_data(data, first_cell_is_header) diff --git a/app/components/govuk_component/table_component/head_component.html.erb b/app/components/govuk_component/table_component/head_component.html.erb deleted file mode 100644 index 70d6173a..00000000 --- a/app/components/govuk_component/table_component/head_component.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -<%= tag.thead(**html_attributes) do %> - <% rows.each do |row| %> - <%= row %> - <% end %> -<% end %> diff --git a/app/components/govuk_component/table_component/head_component.rb b/app/components/govuk_component/table_component/head_component.rb index 90a3e021..1dd843b3 100644 --- a/app/components/govuk_component/table_component/head_component.rb +++ b/app/components/govuk_component/table_component/head_component.rb @@ -17,6 +17,10 @@ def initialize(rows: nil, classes: [], html_attributes: {}) build_rows_from_row_data(rows) end + def call + tag.thead(**html_attributes) { safe_join(rows) } + end + private def build_rows_from_row_data(data) diff --git a/app/components/govuk_component/table_component/row_component.html.erb b/app/components/govuk_component/table_component/row_component.html.erb deleted file mode 100644 index 2a156666..00000000 --- a/app/components/govuk_component/table_component/row_component.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -<%= tag.tr(**html_attributes) do %> - <% cells.each do |cell| %> - <%= cell %> - <% end %> -<% end %> diff --git a/app/components/govuk_component/table_component/row_component.rb b/app/components/govuk_component/table_component/row_component.rb index cb28d444..000b9df4 100644 --- a/app/components/govuk_component/table_component/row_component.rb +++ b/app/components/govuk_component/table_component/row_component.rb @@ -33,6 +33,10 @@ def self.from_body(*args, **kwargs, &block) new(*args, parent: 'tbody', **kwargs, &block) end + def call + tag.tr(**html_attributes) { safe_join(cells) } + end + private def build_cells_from_cell_data(cell_data)