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

Add definition list component for settings screens #1899

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

chrispymm
Copy link
Contributor

This PR adds a new component to make rendering lists for settings screens easier / more consistent.

Instead of this:

<dl class="fb-settings-list">
  <div>
    <dt><%= link_to t('settings.form_information.heading'), settings_form_information_index_path(service.service_id), class: 'govuk-link' %></dt>
    <dd class="govuk-hint"><%= t('settings.form_information.lede') %></dd>
  </div>
  ...
</dl>

we can write this:

<%= mojf_definition_list do |c| %>
  <% c.with_items([
      {
        title: t('settings.form_information.heading'),
        href: settings_form_information_index_path(service.service_id),
        description: t('settings.form_information.lede')
      },
      ...
]) %>
<% end %>

Instead of using with_items it is also possible to use standard block content within the DefinitionListComponent and manually render DefinitionListItemComponents e.g.

<%= mojf_definition_list do |c| %>
  <%= mojf_definition_list_item(
    title: 'Title',
    href: 'http://google.com',
    description: 'A description of the list item')
  %>
<% end %>

The DefinitionListItemComponent also takes a display parameter, giving the ability to hide settings items that are behind feature flags:

<%= mojf_definition_list_item(
    title: 'Title',
    href: 'http://google.com',
    description: 'A description of the list item'),
    display: ENV('feature') == 'enabled'
  %>

Questions:

  1. Is this an improvement?
  2. Does this make writing the UI templates easier from a backend perspective?
  3. There are two methods of writing templates for ViewComponents - either an inline template within the component class or a separate html.erb template - which is our preference?
def call
  tag.dl(**html_attributes) do
    if items?
      safe_join( items.collect { |item| item } )
    else
      content
    end
  end
end

or a separate html template:

#app/components/mojf/definition_list_component.html.erb
<%= tag.dl(**html_attributes) do %>
  <% if items? %>
    <% items.each do |item| %>
      <%= item %>
    <% end %>
  <% else %>
    <%= content %>
  <% end %>
<% end %>

I have opted for the inline method here (mostly for brevity and simplicity) is this the preferred method? Is it easier to understand / reaosn about having it in a more ruby style? Or would we prefer a more 'classic' html.erb template?

@njseeto
Copy link
Contributor

njseeto commented Nov 25, 2022

Sorry Chris, I've missed this PR. To answer the questions:

  1. I think the introduction of these classes makes it much cleaner -a definite improvement
  2. I think it'll make it easier to read and possibly write once we get the hang of it
  3. I'm easy on this one, both are fine with me. The inline option is cleaner but the classic method might be easier to understand on first glance/to pick up

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.

None yet

2 participants