WIP: Formatting test #415

anthmatic · 2022-03-08T23:46:43Z

Types of change

Bug fix (change which fixes an issue)
Enhancement (change which improves upon an existing feature)
New feature (change which adds new functionality)

Description

A clear and concise description of what the pull request is solving.

Relevant links

Example page:
GitHub issue:

Checklist

I have read the CONTRIBUTING document.
My code follows the style guide requirements.
I have thoroughly tested my change.

People to notify

anthmatic · 2022-03-08T23:48:12Z

src/modules/blog-pagination.module/module.html

+  {# First page link #}
+
+  {% if module.show_first_and_last_links == true %}
+  <a class="pagination__link pagination__link--first {{ "pagination__link--disabled" if not last_page_num }}">


Missing template data

anthmatic · 2022-03-08T23:48:22Z

src/modules/blog-pagination.module/module.html

+<nav aria-label="Pagination navigation" role="navigation" class="pagination">
+  {% set page_list =  %}
+  {% if contents.total_page_count - current_page_num == 1 %}
+  {% set offset =  %}


anthmatic · 2022-03-09T00:31:19Z

src/modules/pricing-card.module/module.html

 {% if module.button_link.no_follow %}
-  {% do rel.append("nofollow") %}
+{% dorel.append("nofollow") %}


anthmatic · 2022-03-09T00:31:23Z

src/modules/pricing-card.module/module.html

 {% endif %}
-{% set rel = [] %}
+{% set rel =  %}


anthmatic · 2022-03-09T00:32:14Z

src/modules/pricing-card.module/module.html

+                                        name="{{ module.feature_icon.name }}"
+                                        purpose="decorative"
+                                        style="{{ module.feature_icon.type }}"
+                                        unicode="{{ module.feature_icon.unicode }}" %}{{ feature }}</li>


weird formatting

anthmatic · 2022-03-15T18:29:27Z

src/modules/button.module/module.html

 {% if module.link.no_follow %}
-  {% do rel.append("nofollow") %}
+{% dorel.append("nofollow") %}


space missing

src/modules/button.module/module.html

anthmatic · 2022-03-15T18:31:17Z

src/modules/pricing-card.module/module.html

@@ -4,141 +4,143 @@
  <style>
    {% scope_css %}

-      {# Features #}
+        {# Features #}


I'm thinking scoped css should never add indentation?

I'd be good with that rule 👍🏼 so maybe: indent for if statements but don't indent for HubL functions (e.g. require_css/scope_css) CC: @TanyaScales

anthmatic · 2022-03-15T18:31:17Z

src/modules/pricing-card.module/module.html

@@ -4,141 +4,143 @@
  <style>
    {% scope_css %}

-      {# Features #}
+        {# Features #}


I'm thinking scoped css should never add indentation?

anthmatic · 2022-03-15T18:33:04Z

src/templates/partials/footer.html

+              [ {"social_account": "facebook-f"}, {"social_account": "linkedin-in"},
+              {"social_account": "twitter"}, {"social_account": "instagram"} ]


arrays shouldn't break

anthmatic · 2022-03-15T18:33:04Z

src/templates/partials/footer.html

+              [ {"social_account": "facebook-f"}, {"social_account": "linkedin-in"},
+              {"social_account": "twitter"}, {"social_account": "instagram"} ]


arrays shouldn't break

TanyaScales · 2022-03-17T16:06:36Z

src/modules/blog-listings.module/module.html

-  {% set list_type = "small" %}
+{% set list_type = "small" %}
 {% elif module.layout_style == "list" %}
-  {% set list_type = "large" %}
+{% set list_type = "large" %}


I would still expect the {% set %} lines to be indented inside of the if statement.

Current prettier formatting:

{% if module.layout_style == "card" %} {% set list_type = "small" %} {% elif module.layout_style == "list" %} {% set list_type = "large" %} {% endif %}

Previous and expected format:

{% if module.layout_style == "card" %} {% set list_type = "small" %} {% elif module.layout_style == "list" %} {% set list_type = "large" %} {% endif %}

Curious to hear @jasonnrosa thoughts on this, it appears this is a rule around all {% if %} statements

Yeah I would also expect and much prefer the previous comment. Formatting of the first instance Tanya mentioned is much harder to read. In general we always prefer an indent within if statements.

TanyaScales · 2022-03-17T16:15:05Z

src/modules/button.module/module.html

+    {% if module.link.open_in_new_tab %}target="_blank"
+    {% endif %}
+    {% if rel %}rel="{{ rel|join( ) }}"
+    {% endif %}


This is odd formatting to me -- seems like this should stay on one line, or if following some of the other ways prettier is trying to format, the attribute should be on its own line:

{% if module.link.open_in_new_tab %} target="_blank" {% endif %}

TanyaScales · 2022-03-17T16:19:09Z

src/modules/menu.module/module.html

-  {% set menu = menu(module.menu, "site_root").children %}
+  {% set menu =  %}


Missing/removed the value here

TanyaScales · 2022-03-17T16:20:19Z

src/modules/menu.module/module.html

-    <li class="menu__item menu__item--depth-{{ depth }} {{ "menu__item--has-submenu" if link.children && depth < module.max_levels }} hs-skip-lang-url-rewrite">
+    <li class="menu__item menu__item--depth-depth {{ "menu__item--has-submenu" if link.children and depth < module.max_levels }} hs-skip-lang-url-rewrite">


Seems to have removed the brackets around depth in the class name:

menu__item menu__item--depth-{{ depth }} changed to menu__item menu__item--depth-depth

TanyaScales · 2022-03-17T16:21:32Z

src/modules/menu.module/module.html

+        <span class="menu__child-toggle-icon"></span>
+      </button>
+      <ul class="menu__submenu menu__submenu--level-{{ depth + 1 }} no-list">
+        {% set depth =  %}


Missing/removed depth + 1 here

TanyaScales · 2022-03-17T16:24:47Z

src/modules/menu.module/module.html

-        <a class="menu__link {{ "menu__link--toggle" if link.children && depth < module.max_levels }} {{ "menu__link--active-branch" if link.activeBranch }} {{ "menu__link--active-link" if link.activeNode }}" href="{{ link.url }}" {{ 'aria-haspopup="true" aria-expanded="false"' if link.children && depth < module.max_levels }} {{ 'aria-current="page"' if link.activeNode }} {{ 'target="_blank" rel="noopener"' if link.linkTarget }}>{{ link.label }}</a>
+      <a class="menu__link {{ "menu__link--toggle" if link.children and depth < module.max_levels }} {{ "menu__link--active-branch" if link.activeBranch }} {{ "menu__link--active-link" if link.activeNode }}" href="{{ link.url }}" {{ "aria-haspopup="true" aria-expanded="false"" if link.children and depth < module.max_levels }} {{ "aria-current="page"" if link.activeNode }} {{ "target="_blank" rel="noopener"" if link.linkTarget }}
+        >{{ link.label }}</a
+      >


Bit of strange formatting here, breaking the ending caret of the anchor tag to a new line on both the opening and closing tag

Also noticed that the quotes were changed here from single to double causing this line to break:

Original:

{{ 'target="_blank" rel="noopener"' if link.linkTarget }}

Change to:

{{ "target="_blank" rel="noopener"" if link.linkTarget }}

There are other instances of the single to double quotes as well in this line (and probably throughout this PR)

Ah- Good catch! Will look into this

jasonnrosa · 2022-03-17T17:12:18Z

src/modules/blog-listings.module/module.html


-    {# Tags #}
+        {# Tags #}


I think we have some extra spacing being added in here

This actually looks like it is across the board (looks like it might be pushing things in in some areas 4 spaces instead of 2).

I've noticed this in another scope_css block, are you noticing this in any other tags?

Normally we indent the body inside of a tag block, but because this is CSS (and we're not really parsing CSS) it might make sense to skip that first indent for scope_css

TanyaScales · 2022-03-17T18:45:18Z

src/modules/pricing-card.module/module.html

-  {% set href = "mailto:" + href %}
+{% set href =  %}


missing here too

TanyaScales · 2022-03-17T18:47:30Z

src/modules/pricing-card.module/module.html

-  <hr class="card__hr">
+  <hr class="card__hr" />


not sure if we want self-closing tags to have their closing backslash removed - @jasonnrosa ?

These are coming straight from prettier and I believe that there is no option to override them. It definitely warrants another look, though.

Yeah I'd prefer to omit if possible but sounds like this obviously depends on prettier

TanyaScales · 2022-03-17T18:51:05Z

src/sections/call-to-action.html

-  },
-  max_width={{ context.max_width || 700 }},


Seems to be removing the commas between the top level params.

Current

background_image={...} max_width={{...}} padding={...} vertical_alignment="MIDDLE"

Expected:

background_image={...}, max_width={{...}}, padding={...}, vertical_alignment="MIDDLE"

jasonnrosa · 2022-03-18T15:18:07Z

src/modules/blog-listings.module/module.html

-      {% endif %}
+  <article
+    class="blog-index__post blog-index__post--{{ list_type }}"
+    aria-label="Blog post summary: {{ content.name }}"


Is there any reason this is pushing attributes to their own line?

The prettier HTML formatter is handling HTML attributes, so this is expected behavior

Also worth noting that prettier is really opinionated and the team does not accept requests for new options. More info on that here https://prettier.io/docs/en/option-philosophy.html

jasonnrosa · 2022-03-18T15:19:05Z

src/modules/blog-listings.module/module.html

 {% endif %}

 {# Blog author listing header #}

 {% if blog_author %}
-  <h2 class="blog-author-heading">{{ module.blog_author_listing.subheading }} {{ blog_author.display_name }}:</h2>
+<h2 class="blog-author-heading">{{ module.blog_author_listing.subheading }} {{ blog_author.display_name }}:</h2>


Similar indenting issue

jasonnrosa · 2022-03-18T15:19:13Z

src/modules/blog-listings.module/module.html

 {% endif %}

 {# Blog tag listing header #}

 {% if tag %}
-  <h2 class="blog-tag-heading">{{ module.blog_tag_listing.subheading }} {{ page_meta.html_title|split(" | ")|last }}:</h2>
+<h2 class="blog-tag-heading">{{ module.blog_tag_listing.subheading }} {{ page_meta.html_title|split(" | ")|last }}:</h2>


Similar indenting issue

jasonnrosa · 2022-03-18T15:23:47Z

src/modules/pricing-card.module/module.html

 {% endif %}
 {% if module.button_link.open_in_new_tab %}
-  {% do rel.append("noopener") %}
+{% dorel.append("noopener") %}


jasonnrosa · 2022-03-18T15:26:09Z

src/modules/social-follow.module/module.html

-      {% do rel.append("nofollow") %}
+  {# Sets attributes used for the link field #}
+
+  {% set href =  %}


jasonnrosa · 2022-03-18T15:26:21Z

src/modules/social-follow.module/module.html

+
+  {% set href =  %}
+  {% if item.social_link.url.type == "EMAIL_ADDRESS" %}
+  {% set href =  %}


jasonnrosa · 2022-03-18T15:26:28Z

src/modules/social-follow.module/module.html

+  {% if item.social_link.url.type == "EMAIL_ADDRESS" %}
+  {% set href =  %}
+  {% endif %}
+  {% set rel =  %}


jasonnrosa · 2022-03-18T15:26:36Z

src/modules/social-follow.module/module.html

+  {% endif %}
+  {% set rel =  %}
+  {% if item.social_link.no_follow %}
+  {% dorel.append("nofollow") %}


jasonnrosa · 2022-03-18T15:26:42Z

src/modules/social-follow.module/module.html

+  {% dorel.append("nofollow") %}
+  {% endif %}
+  {% if item.social_link.open_in_new_tab %}
+  {% dorel.append("noopener") %}


jasonnrosa · 2022-03-18T15:27:13Z

src/modules/social-follow.module/module.html

+  {% set social_icon =  %}
+  {% else %}
+  {% set social_icon =  %}
+  {% endif %}


missing values for two social_icon variables above

jasonnrosa · 2022-03-18T15:28:25Z

src/sections/call-to-action.html

-  vertical_alignment="MIDDLE"
-%}
+  }
+  vertical_alignment="MIDDLE" %}


Doesn't seem to be allowing the closing tag being on its own line.

jasonnrosa · 2022-03-18T15:28:48Z

src/sections/call-to-action.html

      {% end_dnd_module %}
    {% end_dnd_row %}
    {% dnd_row %}
      {% dnd_module
-        path="../modules/button",
-        button_text={{ context.button_text || "Get started" }},
+        path="../modules/button"


removed comma

jasonnrosa · 2022-03-18T15:29:31Z

src/sections/hero-banner.html

+      "src": context.image or get_asset_url("../images/grayscale-mountain.png")
+    }
+    offset=0
+    width=6 %}


More missing commas above

jasonnrosa · 2022-03-18T15:29:56Z

src/sections/hero-banner.html

-    offset=6,
-    width=6
-  %}
+  {% dnd_column offset=6 width=6 %}


Shifted these from being on their own lines with commas to being in one line with no comma

Fixing issue with commas.

In regards to going from multiple lines to single line, prettier will try to fit a tag on one line and only break if it doesn't fit. Curious if folks feel strongly about always breaking on new lines?

We typically always have HubL tags break params onto their own lines throughout all of our assets -- so, I generally do appreciate that formatting -- what I would like more is consistency -- so if you're saying that some HubL tags will break onto multiple lines because they are longer/have more params, and others won't because they are shorter, I don't really like them diverging like that

Yeah I also much prefer them broken out onto multiple lines. I find it a lot easier to read separate parameters on their own lines vs. trying to read them all on one line (similar to CSS declarations).

jasonnrosa · 2022-03-18T15:32:51Z

src/templates/hubdb.html

            pageEditor.appendChild(linkNode);
          }
        </script>
-      {% end_require_js %}
+  {% end_require_js %}


Would just want to make sure all of this JS formatting is expected

jasonnrosa · 2022-03-18T15:33:18Z

src/templates/layouts/base.html

    {{ require_css(get_asset_url("../../css/main.css")) }}
-    {# This is intended to be used if a template requires template specific style sheets #}
+    {#
+    <!-- This is intended to be used if a template requires template specific style sheets -->


Added HTML comment within HubL comment

jasonnrosa · 2022-03-18T15:33:42Z

src/templates/layouts/base.html

    {% endif %}
    {{ require_css(get_asset_url("../../css/theme-overrides.css")) }}
-    {# To see a full list of what is included via standard_header_includes please reference this article: https://developers.hubspot.com/docs/cms/hubl/variables#required-page-template-variables #}
+    {#
+    <!-- To see a full list of what is included via standard_header_includes please reference this article: https://developers.hubspot.com/docs/cms/hubl/variables#required-page-template-variables -->


HTML within HubL comment

anthmatic added 8 commits March 7, 2022 06:38

replace operators

d083234

more hubl fixes

c04a42a

replace missing template code

c16374c

adjust spacing

48284cc

fixing alignment

fd7e700

use not instead of !

3638937

more changes

7b3224c

first example pass

8a4b67e

anthmatic commented Mar 8, 2022

View reviewed changes

anthmatic commented Mar 9, 2022

View reviewed changes

src/modules/pricing-card.module/module.html

{% endif %}

{% set rel = [] %}

{% set rel = %}

Copy link

Contributor Author

anthmatic Mar 9, 2022

Choose a reason for hiding this comment

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

missing

anthmatic commented Mar 9, 2022

View reviewed changes

anthmatic commented Mar 15, 2022

View reviewed changes

src/modules/button.module/module.html Show resolved Hide resolved

anthmatic commented Mar 15, 2022

View reviewed changes

TanyaScales reviewed Mar 17, 2022

View reviewed changes

jasonnrosa reviewed Mar 17, 2022

View reviewed changes

TanyaScales reviewed Mar 17, 2022

View reviewed changes

jasonnrosa reviewed Mar 18, 2022

View reviewed changes

		[ {"social_account": "facebook-f"}, {"social_account": "linkedin-in"},
		{"social_account": "twitter"}, {"social_account": "instagram"} ]

		{% set menu = menu(module.menu, "site_root").children %}
		{% set menu = %}

		<li class="menu__item menu__item--depth-{{ depth }} {{ "menu__item--has-submenu" if link.children && depth < module.max_levels }} hs-skip-lang-url-rewrite">
		<li class="menu__item menu__item--depth-depth {{ "menu__item--has-submenu" if link.children and depth < module.max_levels }} hs-skip-lang-url-rewrite">

WIP: Formatting test #415

Are you sure you want to change the base?

WIP: Formatting test #415

Conversation

anthmatic commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment