Skip to content
This repository has been archived by the owner. It is now read-only.

Fix postal address print styles #35

Merged
merged 2 commits into from Oct 30, 2015
Merged

Fix postal address print styles #35

merged 2 commits into from Oct 30, 2015

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Oct 29, 2015

When printing, each part of the address was appearing on a single line, and the first line appeared above it as a header. Anyone printing these pages and then transcribing the address would likely send things to the wrong place.

  • Don’t use styled spans, use divs (empty divs will have a zero height, whereas spans with line breaks would create new lines for empty parts)
  • Don’t use a heading for the title
  • Bump govuk_frontend_toolkit

https://trello.com/c/jT7QTzBb/145-fix-print-styles-on-addresses-medium

Before

screen shot 2015-10-29 at 16 00 30

## After

screen shot 2015-10-29 at 16 00 14

fofr added 2 commits Oct 29, 2015
The prior version was over 18 months old.
When printing, each part of the address was appearing on a single line,
and the first line appeared above it as a header. Anyone printing these
pages and then transcribing the address would likely send things to the
wrong place.

* Don’t use styled spans, use divs (empty divs will have a zero height,
whereas spans with line breaks would create new lines for empty parts)
* Don’t use a heading for the title
@boffbowsh
Copy link
Contributor

@boffbowsh boffbowsh commented Oct 29, 2015

I assume there's no change to how this is rendered on the screen? Is the removal of the h3 harmful to screen readers?

@fofr
Copy link
Contributor Author

@fofr fofr commented Oct 29, 2015

There's no change to the display, the h3 styles were being reset. This brings markup closer inline with how we render contacts elsewhere on GOV.UK. Addresses ought to be displayed using a component.

Regarding screen readers, the first line of the address is part of the address not the heading for the address. If anything the previous heading was confusing. There isn't something that precedes these lines that says "this is a mailing/postal address" other than the "Post" heading a bit higher up. This would be something worth investigating when building out the component. cc @dsingleton

@boffbowsh
Copy link
Contributor

@boffbowsh boffbowsh commented Oct 29, 2015

LGTM 👍 but I won't merge as IANAF

@robinwhittleton
Copy link

@robinwhittleton robinwhittleton commented Oct 30, 2015

I’m not too fussed, but personally I would have left it as spans, removed the span styling and added a <br/> inbetween each line (see http://www.w3.org/TR/html51/semantics.html#the-br-element). So:

<span class="fn"><%= post_address.title %></span>
<br><span class="street-address"><%= post_address.street_address %></span>
<br><span class="locality"><%= post_address.locality %></span>
<br><span class="region"><%= post_address.region %></span>
<br><span class="postal-code"><%= post_address.postal_code %></span>
<% if post_address.world_location.present? %>
  <br><span class="country-name"><%= post_address.world_location %></span>
<% end %>

This works as well though, there’s no accessibility issues that I can think of.

@fofr
Copy link
Contributor Author

@fofr fofr commented Oct 30, 2015

@robinwhittleton I tried that first although without extra logic it can leave gaps between lines in addresses for those that don't contain all parts.

boffbowsh added a commit that referenced this pull request Oct 30, 2015
Fix postal address print styles
@boffbowsh boffbowsh merged commit dc96279 into master Oct 30, 2015
1 check passed
1 check passed
default "Build #67 succeeded on Jenkins"
Details
@jamiecobbett jamiecobbett deleted the fix-address-print-styles branch Oct 30, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.