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

Update search component #2462

Merged
merged 4 commits into from
Nov 24, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
useful summary for people upgrading their application, not a replication
of the commit log.

## Unreleased

* Update search component ([PR #2462](https://github.com/alphagov/govuk_publishing_components/pull/2462))

## 27.14.1

* Remove redundant API value from big number component ([PR #2459](https://github.com/alphagov/govuk_publishing_components/pull/2459))
Expand Down
34 changes: 25 additions & 9 deletions app/views/govuk_publishing_components/components/_search.html.erb
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
<%
shared_helper = GovukPublishingComponents::Presenters::SharedHelper.new(local_assigns)
heading_helper = GovukPublishingComponents::Presenters::HeadingHelper.new(local_assigns)

aria_controls ||= nil
button_text ||= t("components.search_box.search_button")
id ||= "search-main-" + SecureRandom.hex(4)
wrap_label_in_a_heading ||= false
label_margin_bottom ||= nil
label_size ||= nil
label_text ||= t("components.search_box.label")
name ||= "q"
Expand All @@ -14,13 +17,6 @@
data_attributes ||= {}
data_attributes[:module] = 'gem-track-click'

label_classes = []
if (shared_helper.valid_heading_size?(label_size))
label_classes << "govuk-label govuk-label--#{label_size}"
else
label_classes << "gem-c-search__label"
end

classes = %w[gem-c-search govuk-!-display-none-print]
classes << (shared_helper.get_margin_top)
classes << (shared_helper.get_margin_bottom) if local_assigns[:margin_bottom]
Expand All @@ -32,11 +28,31 @@
classes << "gem-c-search--on-white"
end
classes << "gem-c-search--separate-label" if local_assigns.include?(:inline_label) or local_assigns.include?(:label_size)
Copy link
Contributor

@owenatgov owenatgov Nov 23, 2021

Choose a reason for hiding this comment

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

A teeny tiny nitpick: you've mentioned why it's a bad idea to specify the inline_label using the ||= convention at the top (because it's falsey, ruby will defer to the default value). Do you think its worth adding a comment to this effect? I'm thinking of a well-meaning dev seeing that it's not included at the top and adding it in, potentially breaking a few things.

I'm asking instead of suggesting partly because the tests would catch this and partly to document this quirk of ruby in our PR history.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, this is a can of worms. I agree, it's a thing that might be overridden without someone meaning to - so we could suggest moving away from using false values as the default.

For this component, I'd also suggest that renaming the inline_label to label_position of which the options are inline or above (or something similar, just throwing ideas out here). Then we can set the default to be inline using the ||=, run the two parameters in parallel while we update the component usage, and then deprecate it. I think there are a couple of breaking changes coming up, which is good timing.

I'll raise an issue to gently nudge a discussion around this and see what the community consensus is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue raised: #2472


label_classes = []
if (shared_helper.valid_heading_size?(label_size))
label_classes << "govuk-label govuk-label--#{label_size}"
else
label_classes << "gem-c-search__label"
end
label_classes << "govuk-!-margin-bottom-#{label_margin_bottom}" if [*1..9].include?(label_margin_bottom) and local_assigns.include?(:inline_label)

tag_label = capture do
tag.label({ for: id, class: label_classes }) do
label_text
end
end
%>

<div class="<%= classes.join(" ") %>" data-module="gem-toggle-input-class-on-focus">
<%= tag.label({ for: id, class: label_classes }) do %>
<%= label_text %>
<% if wrap_label_in_a_heading %>
<%= content_tag(shared_helper.get_heading_level, {
class: "govuk-!-margin-0",
}) do %>
<%= tag_label %>
<% end %>
<% else %>
<%= tag_label %>
<% end %>
<div class="gem-c-search__item-wrapper">
<%= tag.input(
Expand Down
25 changes: 25 additions & 0 deletions app/views/govuk_publishing_components/components/docs/search.yml
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,28 @@ examples:
Allows the label text size to be set to `xl`, `l`, `m`, or `s`. If this is set, then `inline_label` is automatically set to `false`.
data:
label_size: "xl"
wrap_label_inside_a_heading:
description: |
Puts the label inside a heading; heading level defaults to 2 if not set.

(The size of the label can still be set with `label_size` to appear more like a heading.)
data:
wrap_label_in_a_heading: true
heading_level: 1
with_margin_bottom:
description: |
Allows the spacing at the bottom of the component to be adjusted.

This accepts a number from 0 to 9 (0px to 60px) using the [GOV.UK Frontend spacing scale](https://design-system.service.gov.uk/styles/spacing/#the-responsive-spacing-scale). It defaults to having no margin bottom.
data:
margin_bottom: 9
with_margin_bottom_for_the_label:
description: |
Allows the spacing between the label and the input be adjusted.

Requires `inline_label` to be false.

This accepts a number from 0 to 9 (0px to 60px) using the [GOV.UK Frontend spacing scale](https://design-system.service.gov.uk/styles/spacing/#the-responsive-spacing-scale). It defaults to having no margin bottom.
data:
label_margin_bottom: 9
inline_label: false
47 changes: 47 additions & 0 deletions spec/components/search_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ def component_name
it "renders a search box with default options" do
render_component({})
assert_select ".gem-c-search.gem-c-search--on-white"
assert_select "label[class^='govuk-\!-margin-bottom-']", count: 0
end

it "renders a search box for a dark background" do
Expand Down Expand Up @@ -124,4 +125,50 @@ def component_name
assert_select "input[enterkeyhint='search']", count: 1
assert_select "button[enterkeyhint='search']", count: 1
end

it "has the correct label margin" do
render_component({
inline_label: false,
label_margin_bottom: 9,
})
assert_select 'label.govuk-\!-margin-bottom-9', count: 1
end

it "doesn't set a margin override if label_margin_bottom set to 0" do
render_component({
inline_label: false,
label_margin_bottom: 0,
})
assert_select 'label.govuk-\!-margin-bottom-0', count: 0
end

it "defaults to no bottom margin if an incorrect value is passed" do
render_component({
inline_label: false,
margin_bottom: 20,
})
assert_select "label[class^='govuk-\!-margin-bottom-']", count: 0
end

it "defaults to no bottom margin if inline_label is not passed" do
render_component({
margin_bottom: 2,
})
assert_select "label[class^='govuk-\!-margin-bottom-']", count: 0
end

it "wraps the label in a heading level 2 by default" do
render_component({
wrap_label_in_a_heading: true,
})
assert_select 'h2.govuk-\!-margin-0 > label', count: 1
end

it "wraps the label in the set heading level" do
render_component({
wrap_label_in_a_heading: true,
heading_level: 6,
})
assert_select 'h6.govuk-\!-margin-0 > label', count: 1
end
end