Follow consistent ERB indent style

[davidstosik]

In addition to setting up ERB-lint, these rules will need to be looked after manually, for now.

[jesseclark]

I much prefer this style:

      <!-- ERB tag opener & closer on their own line -->
      <%=
        render "accounts/header",
          left_navigation: link_to(...),
          dismissable: true
      %>

over

      <%= render "accounts/header",
            left_navigation: link_to(...),
            dismissable: true %>

It seems to me that the former is more along the lines of how indentation of other multi-line blocks are handled in our Ruby style, e.g:

# method parameter lists like this

   expect(Sms::TestClient.deliveries.last).to eq(
     phone_number: "441134960000",
     template: "phone_number_verification_code"
   )

# instead of

    expect(Sms::TestClient.deliveries.last).to eq(
      phone_number: "441134960000",
      template: "phone_number_verification_code")

# hashes/arrays like this
{
  foo: bar,
  baz: bash
}

# instead of 

{ foo: bar,
   baz: bash }

Also, it looks like ERBLint::Linters::ClosingErbTagIndent allows both single line tags or ERB tags that open and close on separate lines:

    context 'when tag is correct' do
      let(:file) { "<% foo %>" }
      it { expect(subject).to(eq([])) }
    end

    context 'when tag starts and ends with a newline' do
      let(:file) { <<~ERB }
        <%
          foo
        %>
      ERB
      it { expect(subject).to(eq([])) }
    end

I would prefer that we either go with the style enforced by the linter or allow both.

[davidstosik]

@jesseclark Thank you for the detailed opinion, and sorry for the delay in acknowledging it! 🙇‍♂️

Starting from the end, ERBLint::Linters::ClosingErbTagIndent allows all the following syntaxes:

<%= "1. opening and closing do not use their own new line" %>

<%= "2. opening and closing do not use their own new line,
     this time with a multiline content" %>

<%=
  "3. opening and closing are each on their own new line"
%>

<%=
  "4. opening and closing are each on their own new line,
    this time with a multiline content"
%>

It prevents these:

<%= "only closing is on a new line"
%>

<%=
"only opening is on a new line" %>

<%=
  "opening and closing are each on their own new line, but have a different indentation"
  %>

This entry I propose to add to our guidelines is not an offense to ERBlint, but rather a restriction to the options 1 and 2 above. There are a few reasons why we opted to propose this one after deliberating with @balvig:

• a single rule that can apply to all use-cases is easier remembered and applied than a set of rules that depend on multiple conditions

• for a tag that fits on a single line, we prefer opening and closing brackets to be on the same line:

  <!-- Bad -->
  <%=
    link_to recipe.title, recipe_path(recipe)
  %>
  
  <!-- Good -->
  <%= link_to recipe.title, recipe_path(recipe) %>

• we also had to consider tags that take a block, and felt that the here proposed syntax feels more natural , especially when the do part spreads over multiple lines:

  <!-- Bad -->
  <%=
    link_to_recipe_or_translation recipe,
      class: "media",
      data: {
        "search-recipe-id": recipe.id,
        action: "source-tracking#appendParam search#trackClick intercept-banner#showUrlIntercept"
      } do
  %>
    <%=
      tofu_image_tag(
        recipe.image,
        size: "136x191",
        quality: :low,
        alt: recipe.title,
        filename: recipe.image_filename,
        lazy: true
      )
    %>
  <% end %>
  
  <!-- Good -->
  <%= link_to_recipe_or_translation recipe,
        class: "media",
        data: {
          "search-recipe-id": recipe.id,
          action: "source-tracking#appendParam search#trackClick intercept-banner#showUrlIntercept"
        } do %>
    <%= tofu_image_tag(
          recipe.image,
          size: "136x191",
          quality: :low,
          alt: recipe.title,
          filename: recipe.image_filename,
          lazy: true
        ) %>
  <% end %>

  Note this PR does not impose any white spacing rule, so I'd personally recommend making use of more empty lines to make the above more readable:

  <!-- Better -->
  <%= link_to_recipe_or_translation recipe,
        class: "media",
        data: {
          "search-recipe-id": recipe.id,
          action: "source-tracking#appendParam search#trackClick intercept-banner#showUrlIntercept"
        } do %>
  
    <%= tofu_image_tag(
          recipe.image,
          size: "136x191",
          quality: :low,
          alt: recipe.title,
          filename: recipe.image_filename,
          lazy: true
        ) %>
  
  <% end %>

I'll admit that I myself prefer the option 4 over option 2 when its content is not taking a block (no do/end), but looking for uniformity and simplicity, the proposed rule made more sense.

[balvig]

@davidstosik following up on https://github.com/cookpad/global-web/pull/20353, some suggestions: https://github.com/cookpad/global-style-guides/compare/ds%2Ffollow-consistent-erb-indent-style...suggestion-9df29f87e5c36130 🙇

[davidstosik]

♻️

[balvig]

Well merge this on @davidstosik's behalf