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.

Sign up for GitHub

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

Jump to bottom

Prevent empty heading in contact form #1515

Merged
merged 6 commits into from Mar 29, 2022
Merged

Prevent empty heading in contact form #1515

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b6f3609
Only render contact heading if not empty
kmeleta Mar 21, 2022
d7e03f6
Add fallback form heading
kmeleta Mar 24, 2022
1a6f10e
Move title string
kmeleta Mar 24, 2022
696ad7c
Update 21 translation files
translation-platform[bot] Mar 25, 2022
330db87
Update 3 translation files
translation-platform[bot] Mar 25, 2022
7616c28
Update 6 translation files
translation-platform[bot] Mar 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion sections/contact-form.liquid
View file
Open in desktop
Expand Up @@ -16,7 +16,9 @@

<div class="color-{{ section.settings.color_scheme }} gradient">
<div class="contact page-width page-width--narrow section-{{ section.id }}-padding">
<h2 class="title title-wrapper--no-top-margin {{ section.settings.heading_size }}">{{ section.settings.heading | escape }}</h2>
{%- if section.settings.heading != blank -%}
<h2 class="title title-wrapper--no-top-margin {{ section.settings.heading_size }}">{{ section.settings.heading | escape }}</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

On "Contact Us" templates only, remove the empty h2 element from the DOM. Keep the heading for "Contact Us" sections.

This contact-form.liquid file is used for the contact form section and the contact page template.
Adding this condition will remove the heading for sections and for page templates.

Something to consider is that there is no guarantee that a merchant will use the Contact us Title for the contact us page. The contact us template can also be assigned to multiple pages, other than the contact us page.

We could consider using a hidden h2 for sections when a merchant doesn't include a heading for the section.

<h2 class="visually-hidden">Contact us</h2>

^ This may be more verbose for a Contact us page assigned to the the contact us template. That said, this section can be added to other templates, unrelated to the contact us page, and it could be useful to add the hidden h2 to make the purpose of the form clearer.

Copy link
Member

Choose a reason for hiding this comment

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

We could consider using a hidden h2 for sections when a merchant doesn't include a heading for the section.

Agreed. As is the same solution suggested in #1457.

Copy link
Contributor Author

@kmeleta kmeleta Mar 21, 2022 •
edited

Choose a reason for hiding this comment

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

I see, yeah that makes sense. Would "Contact form" be acceptable language? I think this already exists as a translated string, where as "Contact us" does not, far as I'm aware.

edit: I'm wrong. I'd need to add one anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could consider using a hidden h2 for sections when a merchant doesn't include a heading for the section.

Scott, just to clarify for Ken: you are aligned with always including the hidden h2 on templates and sections if no heading is included?

So instead of:

{%- if section.settings.heading != blank -%}
      <h2 class="title title-wrapper--no-top-margin {{ section.settings.heading_size }}">{{ section.settings.heading | escape }}</h2>
{%- endif -%}

We would use something like:

{% assign heading = section.settings.heading | default: "Contact us" %}
 <h2 class="{% if section.settings.heading %}title title-wrapper--no-top-margin {{ section.settings.heading_size }{% else %}visually-hidden{% endif %}">
  {{ heading | escape }}
</h2>

Copy link
Member

Choose a reason for hiding this comment

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

The verbosity concern is fair. Is there a way to detect section vs template? If not no worries. I feel the need for a visually hidden h2 when a section title is empty outweighs the duplicate h1&h2 text on the contact page. It may be a little confusing at first but this also resolves the empty heading error detected.

Copy link
Contributor

Choose a reason for hiding this comment

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

detect section vs template

Short answer: Yes.
We know that the contact-us.liquid is a section. It will always be used as a section.

We can check if a template is a page. For example, request.page_type can tell us that the template is a page. We can also check the template name with template.name --> page.contact. However, because the contact us page template can be assigned to multiple pages, I would recommend the duplicate heading, instead of conditionally removing it for this page template (Contact us).

If we use Contact form as the heading (as Ken suggested), there is less likely to be exactly the same heading as the page title, and would be useful in other template contexts. It's not ideal, but like you said "the need for a visually hidden h2 when a section title is empty outweighs the duplicate h1&h2 text on the contact page".

Copy link
Member

Choose a reason for hiding this comment

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

If we use Contact form as the heading (as Ken suggested), there is less likely to be exactly the same heading as the page title, and would be useful in other template contexts.

Good point. Let's roll with this.

{%- endif -%}
{%- form 'contact', id: 'ContactForm', class: 'isolate' -%}
{%- if form.posted_successfully? -%}
<div class="form-status form-status-list form__message" tabindex="-1" autofocus>{% render 'icon-success' %} {{ 'templates.contact.form.post_success' | t }}</div>
Expand Down