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

Fix inline attachment rendering #5237

Merged
merged 2 commits into from Jan 7, 2020
Merged

Conversation

@vanitabarrett
Copy link
Contributor

vanitabarrett commented Jan 7, 2020

What

Removes trailing newlines from inline attachment view. There were two things causing the additional space:

  • The closing span tag being on a newline
  • The inline_attachment file having a newline at the end of the file (part of our standards)

Why

  • Inline attachments were rendering with an additional space after the inline attachment.
  • Inline attachments in numbered lists displayed on a new line and showed a closing </span> tag on the page

Before

Screenshot 2020-01-07 at 15 30 11

After

Screenshot 2020-01-07 at 15 29 57

Before

Screenshot 2020-01-07 at 15 52 18

After

Screenshot 2020-01-07 at 15 45 57

brucebolt added 2 commits Nov 8, 2019
There is a bug where an additional space is being added after the name
of an inline attachment, e.g.

```
Download and fill in the [InlineAttachment:1].
```

renders as

```
Download and fill in the registration form (ODS, 16.9KB) .
```

This test will allow us to find the problem.
The repeated spaces following the new line characters are being
rendered by web browsers as a single space, leading to:

```
Download and fill in the [InlineAttachment:1].
```

being rendered as

```
Download and fill in the registration form (ODS, 16.9KB) .
```
@vanitabarrett vanitabarrett changed the title Fix inline attachment rendering Fix additional space after inline attachment Jan 7, 2020
@vanitabarrett vanitabarrett requested review from brucebolt and cbaines Jan 7, 2020
@vanitabarrett vanitabarrett changed the title Fix additional space after inline attachment Fix inline attachment rendering Jan 7, 2020
<%= link_to attachment.title, attachment.url %>
(<%= attachment_attributes(attachment) %>)
</span>
<span id="attachment_<%= attachment.id %>" class="attachment-inline"><%= link_to attachment.title, attachment.url %> (<%= attachment_attributes(attachment) %>)</span>

This comment has been minimized.

Copy link
@huwd

huwd Jan 7, 2020

In some ways this is a bit of a shame to put this on one line.
I find it less readable than what was there previously.

But i guess you need it this way as spans preserve line breaks?
🤔 and a helper seems OTT...

This comment has been minimized.

Copy link
@vanitabarrett

vanitabarrett Jan 7, 2020

Author Contributor

Yeah, I tried just using chomp and putting it back over multiple lines, but both contribute to the additional space, and it's such a small file that anything else seemed like overkill.

@huwd
huwd approved these changes Jan 7, 2020
@vanitabarrett vanitabarrett merged commit 6076e23 into master Jan 7, 2020
3 checks passed
3 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/publishing-e2e-tests Publishing end-to-end tests succeeded on Jenkins
Details
continuous-integration/jenkins/security No security issues found
Details
@vanitabarrett vanitabarrett deleted the fix-inline-attachment-render branch Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.