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 footnotes in legislative lists #216

Merged
merged 4 commits into from
Sep 13, 2021
Merged

Conversation

deborahchua
Copy link
Contributor

What

This fixes an issue with footnotes within legislative lists in Whitehall.
Trello card: https://trello.com/c/yfeqz1no/2561-footnotes-dont-work-within-legislativelist-on-manuals-publisher-5

Why

This has been a longstanding issue and is heavily used in legislation papers.
It means that publishers cannot use footnotes within certain kinds of content, and so cannot always follow the design guidelines. Having markdown that's reliable and effective is helpful for selling our accessibility approach (avoiding PDFs) to policy colleagues.

Visual Changes

Before (footnotes in a legislative list not converted into html):
Screenshot 2021-09-06 at 12 39 21

After (footnotes in a legislative list now showing):
Screenshot 2021-09-06 at 12 40 29

Before (footnote definitions missing):
Screenshot 2021-09-06 at 12 44 13

After (footnote definitions now showing):
Screenshot 2021-09-06 at 12 48 05

@deborahchua deborahchua marked this pull request as ready for review September 6, 2021 12:52
@deborahchua deborahchua changed the title Add footnotes legislative lists Fix footnotes in legislative lists Sep 6, 2021
Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

Well done for working through this fiddly issue! I've got some code style comments below, but otherwise this looks basically ready to go. However, there is something I'd like to check first. From the first commit:

We currently don't have the ability to render footnotes in Legislative
lists. This workaround substitutes the footnote anchor links (e.g. [^1]
for it's equivalent html.

Do we know why we don't have the ability to render footnotes in Legistlative lists? Is there just a method call we need to invoke somewhere? It feels awfully cumbersome to have to recreate our own footnotes/acronyms markdown parsing logic if it already exists and works elsewhere. Apologies if you've already looked into this, I'm viewing this with fresh eyes and haven't looked too closely at the coal face.

My only other question was whether we're duplicating any of this code - it particularly feels as though the "go to where this is referenced" bit should be encapsulated in one place rather than repeated here.

lib/govspeak.rb Outdated Show resolved Hide resolved
lib/govspeak.rb Outdated Show resolved Hide resolved
lib/govspeak.rb Outdated Show resolved Hide resolved
lib/govspeak.rb Show resolved Hide resolved
lib/govspeak.rb Outdated Show resolved Hide resolved
lib/govspeak.rb Outdated Show resolved Hide resolved
@deborahchua
Copy link
Contributor Author

Thanks @ChrisBAshton! Addressing your comments below:

We currently don't have the ability to render footnotes in Legislative
lists. This workaround substitutes the footnote anchor links (e.g. [^1]
for it's equivalent html.

Do we know why we don't have the ability to render footnotes in Legistlative lists? Is there just a method call we need to invoke somewhere? It feels awfully cumbersome to have to recreate our own footnotes/acronyms markdown parsing logic if it already exists and works elsewhere. Apologies if you've already looked into this, I'm viewing this with fresh eyes and haven't looked too closely at the coal face.

So this was the part I spent a great deal of time trying to figure out a better workaround. Leglislative lists was a new markdown tag introduced a while back so that editors can have custom numbering for ordered lists and different indentation levels (see this commit which explains in detail) The approach used means a sub document is created and the footnotes only exists inside that document, with some hacks to the Kramdown to make it work.
Going by the previous comments and my understanding of the problem, this icky workaround seemed like the best option without having to create our own custom markdown parser for lists.

My only other question was whether we're duplicating any of this code - it particularly feels as though the "go to where this is referenced" bit should be encapsulated in one place rather than repeated here.

The html for footnotes doesn't exist at this stage if the document has footnotes in legislative lists, which is why it needs to be manually processed at an earlier point

Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, and for making the changes, @deborahchua! This looks good to go 👍 I've got a couple of nitpick comments below, but they're non-blocking.

lib/govspeak.rb Outdated Show resolved Hide resolved
lib/govspeak.rb Show resolved Hide resolved
We currently don't have the ability to render footnotes in Legislative
lists. This workaround substitutes the footnote anchor links (e.g. [^1]
for it's equivalent html.

In order to know whether the document contains legislative lists
with footnotes we have to check for this during the preprocess stage.
The markdown manually converted to html is then appended or
substituted to the html during the document's post process stage.
The link markdown isn't recognised when within a footnote definition.
This workaround strips the text out, converts it into html separately
and added back in.
In the same way as footnotes, abbreviation markdown isn't recognised
in Legislative lists and also when combined with footnote definitions.

We extract the acronyms and their alt text, and later substitute their
html back into the lists and/or footnote definitions.
@deborahchua deborahchua merged commit 7f93c05 into main Sep 13, 2021
@deborahchua deborahchua deleted the add-footnotes-legislative-lists branch September 13, 2021 10:30
deborahchua added a commit that referenced this pull request Sep 15, 2021
This should now capture all footnote numbers rather than limiting it
to 9 and was missed in #216
deborahchua added a commit that referenced this pull request Sep 15, 2021
This should now capture all footnote numbers rather than limiting it
to 9 and was missed in #216
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