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

Support govspeak in a fieldset legend #178

Closed
wants to merge 2 commits into from
Closed

Conversation

kevindew
Copy link
Member

@kevindew kevindew commented Feb 20, 2018

We cannot put a govspeak component inside a fieldset legend as div is not an allowed child of a legend. This adds an option to the fieldset component which will give govspeak styling to the contents of a legend.

The need for this addition was prompted by this situation we encountered on email-alert-frontend where it was difficult to add styling to the legend.

See example 👉 https://govuk-publishing-compon-pr-178.herokuapp.com/component-guide/fieldset

I'm not hugely convinced this change is a great idea since it means replicating some of the responsibility of the govspeak component, but it seems potentially the best approach in a frustrating situation.

I think on an ideal level it'd be far better to have means to call the styles produced by govspeak by putting classes on the individual elements, but that seems a challenge to reach.

/cc @andysellick

This resolves the issue where this won't install gems in deployment mode
as the gemfile is locked to version 5.1.1 whereas the gem is at 5.1.3.
We cannot put a govspeak component inside a fieldset legend as div is
not an allowed child of a legend. This adds an option to the fieldset
component which will give govspeak styling to the contents of a legend.
@NickColley
Copy link
Contributor

NickColley commented Feb 20, 2018

Hey Kevin,

This is probably not a good idea to reuse internals of the govspeak component.

It's also not designed to wrap components, only markup.

Could you consider doing something like

<% radio = capture do %>
  <%= render "govuk_publishing_components/components/radio", {
    name: "frequency",
    items: [
      {
        value: "immediately",
        text: "As soon as they happen",
        checked: true,
      },
      {
        value: "daily",
        text: "No more than once a day",
      },
      {
        value: "weekly",
        text: "No more than once a week",
      }
    ]
  } %>
<% end %>

<% introText = capture do %>
  <h2>How often do you want to get updates?</h2>
  <p>You can get updates about changes:</p>
<% end %>

<% introGovspeak = capture do %>
  <%= render "govuk_component/govspeak", content: introText %>
<% end %>

<%= render "govuk_publishing_components/components/fieldset", {
  legend_text: introGovspeak,
  text: radio,
} %>

Ideally the govspeak component would take a block so you don't need to capture input to pass to content

Alternatively we should probably have typography classes, since govspeak should only really be needed when you don't have control over the markup.

@kevindew
Copy link
Member Author

Hey @NickColley - the problem we have with that approach is that legend > div is invalid HTML so we can't use the govspeak component in that context if we want our HTML to still be valid. If we could adapt the govspeak component to be available as a span it could work too.

I also don't know if it concerns us too much to have invalid HTML as the title component suggestion in https://govuk-publishing-compon-pr-178.herokuapp.com/component-guide/fieldset/with_html_legend would also produce invalid HTML.

@fofr
Copy link
Contributor

fofr commented Feb 20, 2018

Does the legend content need to be govspeak? Or is it just a heading?

This PR has a similar context:
alphagov/govuk_elements#507

@NickColley
Copy link
Contributor

NickColley commented Feb 20, 2018

Hmm when I implemented https://www.gov.uk/log-in-file-self-assessment-tax-return/sign-in/prove-identity I used the title component.

So seems like it is invalid HTML, but importantly I tested it extensively in an accessibility respect with assistive technology software so I don't think it's actually a problem in practice.

If you were to use the title component also, we could make an update so it uses a span rather than a div.

See source code: https://github.com/alphagov/government-frontend/blob/master/app/views/content_items/service_sign_in/_choose_sign_in.html.erb#L20

@kevindew
Copy link
Member Author

@fofr

Does the legend content need to be govspeak? Or is it just a heading?

Nope, it doesn't need to be (to me it feels a bit wrong that it is). Although rather than it being just the heading though we're relying more on the margins provided by the paragraph: see: https://user-images.githubusercontent.com/282717/36423562-607064f6-1638-11e8-98cc-61a1acedabf6.png

@NickColley If it's not too bad form and there's prior precedent I don't mind using the govspeak component inside, though like said above the paragraph styling is needed as well. We're also using the title component above the form too so need a less high emphasis header.

@NickColley
Copy link
Contributor

@kevindew take a look at that page, I think you can avoid this entirely if you restructure how you're using the fieldset component to use a block.

@kevindew
Copy link
Member Author

@NickColley ok cool, to clarify do you mean?

<%= render "govuk_publishing_components/components/fieldset", {
  legend_text: render("govuk_components/govspeak", content: intro),
  text: radio,
} %>

and just be chill about the HTML?

@NickColley
Copy link
Contributor

NickColley commented Feb 20, 2018

@kevindew title component in the legend, the extra html outside the legend.

e.g.

<%= form_tag() do %>
  <% legend_text = render 'govuk_component/title', title: "How often do you want to get updates?"%>
  <%= render "govuk_publishing_components/components/fieldset", legend_text: legend_text do %>
    <div class="grid-row">
      <div class="column-two-thirds">
        <%= render 'govuk_component/govspeak', content: "<p>You can get updates about changes:</p>" %>
        <% if @error %>
          <%= render "components/error-message", text: t('service_sign_in.error.option') %>
        <% end %>
        <%= render "govuk_publishing_components/components/radio", id_prefix: @content_item.options_id, name: "option", items: @content_item.options %>
      </div>
    </div>
  <% end %>

  <%= render 'govuk_component/button', text: t('service_sign_in.continue'), margin_bottom: true %>
<% end %>

Note: that the title component uses a H1, make sure this is correct for you, if not replace the title component with the govspeak component.

@kevindew
Copy link
Member Author

After a chat with @NickColley I've gone in this direction: alphagov/email-alert-frontend#140 so can close this.

Thanks for help all 👍

@kevindew kevindew closed this Feb 21, 2018
@kevindew kevindew deleted the govspeak-legend branch February 21, 2018 19:14
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 this pull request may close these issues.

4 participants