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

Make it easier to prevent a form_detail_placeholder being printed #2212

Merged
merged 1 commit into from
Aug 20, 2018

Conversation

zarino
Copy link
Member

@zarino zarino commented Aug 17, 2018

Previously, if you didn’t want a hint to appear under the main "details" textarea on the new report form, you had to override the entire form_report.html template, or leave the hint element in the markup but hide it with CSS.

Now, you can set form_detail_placeholder to a falsey value, and the template won’t output the hint element at all. It also amends the aria-describedby attribute on the textarea so it doesn’t end up referencing a hint element that doesn’t exist.

I needed this for mysociety/collideoscope#28, because the existing [% DEFAULT form_detail_placeholder = … %] block gave me no easy way to prevent the hint being shown.

How it looks:

When no custom form_detail_placeholder is defined in the cobrand, eg:

# /templates/web/<cobrand>/report/new/_form_labels.html
[% form_detail_label = loc('Can you describe what happened?') %]

undefined

When a custom form_detail_placeholder is defined in the cobrand, eg:

# /templates/web/<cobrand>/report/new/_form_labels.html
[% form_detail_placeholder = "Helpful cobrand-specific hint here" %]
[% form_detail_label = loc('Can you describe what happened?') %]

truthy

NEW: When form_detail_placeholder is set to a falsey value, eg:

# /templates/web/<cobrand>/report/new/_form_labels.html
[% form_detail_placeholder = 0 %]
[% form_detail_label = loc('Can you describe what happened?') %]

falsey

@zarino zarino requested a review from dracos August 17, 2018 14:41
zarino added a commit to mysociety/collideoscope that referenced this pull request Aug 17, 2018
@codecov
Copy link

codecov bot commented Aug 17, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@f607f21). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2212   +/-   ##
=========================================
  Coverage          ?   79.46%           
=========================================
  Files             ?      182           
  Lines             ?    11830           
  Branches          ?     2206           
=========================================
  Hits              ?     9401           
  Misses            ?     1665           
  Partials          ?      764

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f607f21...0caded8. Read the comment docs.

Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

One possible change to reduce the change is all :)

@@ -49,13 +49,22 @@ <h2 class="form-section-heading">[% loc( 'Public details' ) %]</h2>
[% TRY %][% PROCESS 'report/new/after_photo.html' %][% CATCH file %][% END %]

[% DEFAULT form_detail_label = loc('Explain what’s wrong') %]
[% DEFAULT form_detail_placeholder = loc('e.g. ‘This pothole has been here for two months and…’') %]
Copy link
Member

Choose a reason for hiding this comment

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

What you have works fine, but I wonder if it would be clearer if this line became [% IF NOT form_detail_placeholder.defined %][% SET form_detail_placeholder = ... %][% END %] and then below would be [% IF form_detail_placeholder %]... without needing to duplicate e.g. the aria_describedby line. What do you think? Not much in it.

Previously, if you didn’t want a hint to appear under the main "details"
textarea on the new report form, you had to override the entire
`form_report.html` template, or leave the hint element in the markup but
hide it with CSS.

Now, you can set `form_detail_placeholder` to a falsey value, and the
template won’t output the hint element at all. It also amends the
`aria-describedby` attribute on the textarea so it doesn’t end up
referencing a hint element that doesn’t exist.
@zarino zarino merged commit 0caded8 into master Aug 20, 2018
zarino added a commit to mysociety/collideoscope that referenced this pull request Aug 20, 2018
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.

2 participants