Make image and attachment embedding syntax more consistent #274
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was inconsistent behaviour between the different syntaxes used for embedding images. The `!!n` syntax must start on a new line. But the previous line can contain other content (e.g. paragraph text). For example, this is valid: ``` Some example content, immediately followed by an image. !!1 ``` Whereas the `[Image:]` syntax had to be preceded by two newline characters (i.e. a new Markdown paragraph). For example: ``` This does not work: [Image: example.jpg] But this does because it's preceded by two newline characters: [Image: example.jpg] ``` This inconsistent behaviour was spotted by publishers after we implemented support for the `[Image:]` syntax in Whitehall Publisher, since they're used to the `!!n` syntax which is more lenient. We've decided to bring the two in line by adjusting the `[Image:]` syntax so that it only needs to be preceded by one newline. This still avoids the issue identified in 663e099 because it does not allow images to be embedded within a paragraph. I've tweaked the `[Image:]` regex and adding extra tests to cover the desired behaviour. I've also added equivalent coverage to the `!!n` syntax tests to explicitly call out this behaviour and ensure the two syntaxes behave consistently.
Similar to the previous commit, I'm tweaking the `[Attachment:]` syntax slightly so it supports embedding when preceded by only one newline character. For example: ``` This would not have worked before. [Attachment: example.pdf] Whereas this has always worked: [Attachment: example.pdf] ``` This makes the behaviour of `[Image:]`, `[Attachment:]` and the legacy `!!n` (image) and `!@n` (attachment [in Whitehall][1]) syntaxes consistent. As before, I don't think this introduces any risk of attachments being embedded within paragraphs, since the existing tests which cover that concern still pass. [1]: https://github.com/alphagov/whitehall/blob/1587198a31ae690270a34c27e9df7cb3c8ff8408/test/unit/helpers/govspeak_helper_test.rb#L203-L209
ollietreend
changed the title
Bumps the patch version of the gem. This release fixes a bug/inconsistency but doesn't alter existing behaviour.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
This PR makes changes to the
[Image:]and[Attachment:]embed syntaxes so they behave more consistently with the older!!n(image) and!@n(attachment) syntaxes.In particular,
[Image:]and[Attachment:]only worked when preceded by two newline characters (\n\n). Whereas!!nand!@nonly needed to be preceded by one newline character (\n).Before
For example, this would work when embedding with the
!!nsyntax:But this would not work with the
[Image:]syntax:After
Both examples above work as expected.
They both produce HTML output like:
Why
We've recently introduced support for
[Image:]syntax in Whitehall, and will soon be adding support for[Attachment:]too. We quickly received feedback from users that the new image embedding syntax wasn't working – and it turns out it was because they were used to embedding images using!!nsyntax, which only needed one newline character.To resolve the inconsistency, we've decided to bring the syntaxes in line by making the newer
[Image:]and[Attachment:]syntaxes more lenient as users were expecting.Trello: https://trello.com/c/FxDTL9Ic/1272-update-image-syntax-so-it-behaves-similar-to-images-added-in-whitehall-previously-no-line-break-needed