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

HTML line-break elements not appearing in "contact" containers #55

Open
floehopper opened this issue Jun 23, 2015 · 4 comments
Open

HTML line-break elements not appearing in "contact" containers #55

floehopper opened this issue Jun 23, 2015 · 4 comments

Comments

@floehopper
Copy link
Contributor

According to the relevant section of the README, lines demarcated with $C are supposed to be wrapped in a div with class contact and each line is supposed to be separated by a line-break elements <br />. The wrapping is working as described, but it appears that no line-break elements are being added.

I think this is why the formatting of the Tax Credit Helpline contact details on pages like this one are not formatted as I would expect based on the source govspeak markup

Does anyone know whether this govspeak behaviour is intentional (and the docs are out-of-date) or is the behaviour incorrect?

Note that there looks as if there's specific code in place to add line-breaks for address containers (demarcated by $A).

floehopper added a commit to alphagov/smart-answers that referenced this issue Jun 24, 2015
Although the Govspeak docs [1] suggest that a contact block with generate HTML
line-break elements between each line, this doesn't appear to be the case (I
have raised a GitHub issue [2] asking about this).

It appears as if historically we've been working around this issue by adding
two trailing spaces to the lines within the contact blocks. This relies on a
feature of Kramdown which converts the two trailing spaces into HTML line break
elements [3].

Unfortunately the trailing spaces for the contact blocks in uk-benefits-abroad
were, almost certainly accidentally, lost in this commit [4].

Many developers have their editors set to automatically remove trailing
whitespace to make git history as clear as possible. I think this is generally
a good thing, but I suspect it may have been what led to these significant
trailing spaces being lost.

As a temporary solution, I've added two trailing backslashes to the relevant
lines. This is a Kramdown alternative to the two trailing spaces with the
advantage that hopefully they shouldn't get removed so easily by accident.

I think the correct longer-term solution is probably to fix Govspeak so that
it's behaviour matches the documentation, but since many projects use Govspeak,
it may take a while to get the change accepted.

[1]: https://github.com/alphagov/govspeak#contact
[2]: alphagov/govspeak#55
[3]: http://kramdown.gettalong.org/syntax.html#paragraphs
[4]: 1a96852
@floehopper
Copy link
Contributor Author

I can confirm that the Smart Answers app has been working around this issue by adding two trailing spaces to lines within a contact box. This makes use of Kramdown's line-break mechanism.

As explained in this commit note, it's all too easy for these trailing spaces to get accidentally removed e.g. by a developer's editor automatically stripping trailing whitespace. A slight improvement on this is to use the alternative Kramdown line-break mechanism which involves adding two trailing backslashes - hopefully it should be more obvious that these have been added intentionally.

However, both these solutions feel like workarounds for incorrect behaviour in Govspeak. I'd like to submit a PR to fix the behaviour and bring it in line with the documentation, but before I put the time in, I'd like to be confident that the changes will be accepted. I'm conscious that Govspeak is used by a number of other applications and I wouldn't want to break them.

Does anyone have any thoughts? How do other apps, e.g. Whitehall, deal with this?

@alext
Copy link
Contributor

alext commented Jun 25, 2015

I think that would be a good change to make, however it would be important that this is done in a backwards-compatible manner for markup that already has the manually inserted line-breaks (ie markup with these manual line-breaks shouldn't get an extra line-break as a result of this change.)

@floehopper
Copy link
Contributor Author

@alext: Thanks for your reply. I think that a solution like that might make the code quite complex/brittle. An alternative solution might be to make the new behaviour the default in a new version of the gem, but provide an option to retain the old behaviour. Another benefit of this latter approach would be that any new apps which start using Govspeak would rely on the default/correct behaviour. What do you think?

@alext
Copy link
Contributor

alext commented Jun 25, 2015

I see your point, this will add more complexity to the code.

I'm just aware of the volume of content out there that will be written for the existing behaviour. We don't want to end up in a place where we can't upgrade an app to a newer version of govspeak for fear of breaking existing content.

TBH, both approaches have their issues. I'm not sure which option is best...

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

No branches or pull requests

2 participants