-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Prevent empty heading in contact form #1515
Changes from all commits
b6f3609
d7e03f6
1a6f10e
696ad7c
330db87
7616c28
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,11 @@ | |
|
||
<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> | ||
{%- else -%} | ||
<h2 class="visually-hidden">{{ 'templates.contact.form.title' | t }}</h2> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @martinamarien Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Templates is perfectly fine. |
||
{%- 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> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.^ 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. As is the same solution suggested in #1457.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
We would use something like:
There was a problem hiding this comment.
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 duplicateh1
&h2
text on the contact page. It may be a little confusing at first but this also resolves the empty heading error detected.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 apage
. We can also check the template name withtemplate.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".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Let's roll with this.