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.

Sign up for GitHub

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

Jump to bottom

Implement row level table headings to allow accessible tables with row headings #214

Merged
merged 4 commits into from
Mar 12, 2021
Merged

Implement row level table headings to allow accessible tables with row headings #214

merged 4 commits into from
Mar 12, 2021

Conversation

richardTowers
Copy link
Contributor

Sometimes, tables have row-level headings as well as column level headings.

To be accessible, these need to be marked up as <th> tags. Otherwise users of screen readers and other assistive technology may not be able to work out that they're table headings rather than ordinary table cells.

⚠️ Don't forget to update the gem version in the version file and CHANGELOG before merging! ⚠️

@richardTowers richardTowers mentioned this pull request Mar 8, 2021
Closed
@richardTowers richardTowers force-pushed the table-row-headings branch 2 times, most recently from 211607d to 7af1819 Compare March 9, 2021 10:05
@36degrees
Copy link
Contributor

Looking at vmg/redcarpet@27dfb2a, I'm not sure it actually helps with table row headers – I think it just makes it possible to know whether a given cell is a column header within the context of the table_cell method if you're writing your own renderer?

@richardTowers
Copy link
Contributor Author

Yeah, the problem is if I implement a custom table_cell callback, all the column headers come out as <td>s - there's not enough information in the callback to work out what's a header and what's a cell.

So I could use it to implement row headers, but we'd lose column headers (which work at the moment).

@36degrees
Copy link
Contributor

Ah! I totally mis-interpreted your comment. I was interpreting it as though that commit would bring native row headers to redcarpet, meaning we don't have to do anything at all – rather than that it would allow us to instead use the table_cell callback which would be less hacky.

Makes sense now 👍🏻

@richardTowers
Copy link
Contributor Author

Yep, sorry - I should clarify the comment :)

@richardTowers
Copy link
Contributor Author

Updated to clarify the comment, and to use table_row instead of table, which is a little bit more specific. Test still passes by the looks of it.

@richardTowers richardTowers force-pushed the table-row-headings branch 2 times, most recently from f2a6e60 to 23ae02e Compare March 11, 2021 09:15
richardTowers added a commit to alphagov/tdt-documentation that referenced this pull request Mar 11, 2021
@richardTowers
Document row level headers
31a8914 
This requires alphagov/tech-docs-gem#214 to be
released first - the current version of the tech docs template doesn't
support row headers in markdown tables.

Alternatively we could document that you need to use HTML tables to set
row level headings.
@richardTowers richardTowers mentioned this pull request Mar 11, 2021
Closed
2 tasks
@richardTowers
Implement table row headings
556ef8d 
Sometimes, tables have row-level headings as well as column level
headings.

To be accessible, these need to be marked up as `<th>` tags. Otherwise
users of screen readers and other assistive technology may not be able
to work out that they're table headings rather than ordinary table
cells.
@richardTowers
Add CHANGELOG entry for row level table headings
3de782e
36degrees
36degrees reviewed Mar 11, 2021
View reviewed changes
Copy link
Contributor

@36degrees 36degrees left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense to me. I have got 1 query about the space after the # being required.

I'm also aware it's been a long time since I've written any Ruby, so it might be worth getting a second review from someone who writes Ruby regularly.

lib/govuk_tech_docs/tech_docs_html_renderer.rb Outdated
# Despite the hackiness, this substitution should work for all cases
# where the table cell just includes simple text (with no other markup).

body_with_row_headers = body.gsub(/<td># ([^<]*)<\/td>/, "<th scope=\"row\">\\1</th>")
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right in thinking the space between the # and the text is required for this pattern to match?

From testing, it looks like redcarpet will happily make something a heading without the space (e.g. #Hello world becomes an H1), so I'm wondering if it's worth being consistent on that here.

(Conversely, CommonMark requires the space, but does acknowledge that many implementations don't)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this pattern requires it. I wasn't sure whether it was needed in the spec or not.

🤔 🤔 Not sure whether to keep the space or not...

spec/govuk_tech_docs/tech_docs_html_renderer_spec.rb Outdated
@@ -0,0 +1,35 @@
RSpec.describe GovukTechDocs::TechDocsHTMLRenderer do
describe "#render a table" do
it "renders tables with row headings" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test 💯

@richardTowers
Use nokogiri to process html, instead of regex
c061010 
This allows us to handle the more complex case where table headings
contain other markup (e.g. the heading might have an `<em>` tag, as per
the test).

It feels less hacky than using a regex, but it's quite a bit more code
so I'm not 100% sure whether it's an improvement.
AlanGabbianelli
AlanGabbianelli approved these changes Mar 12, 2021
View reviewed changes
Copy link
Contributor

@AlanGabbianelli AlanGabbianelli left a comment

Choose a reason for hiding this comment

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

I like the use of Nokogiri here. ⭐
It's verbose, true, but it's nice and readable. It's clear what you're doing at each step.
As suggested in our Slack conversation, I'd like to see the test split but that's not a blocker 😉

@richardTowers richardTowers merged commit bff939f into master Mar 12, 2021
@richardTowers richardTowers deleted the table-row-headings branch March 12, 2021 13:04
@AlanGabbianelli
Copy link
Contributor

I see you split the test while I was still writing my comment 😄 Nice

@36degrees 36degrees mentioned this pull request Mar 12, 2021
Closed
3 tasks
This was referenced Mar 13, 2021
Merged
Merged
This was referenced Mar 15, 2021
Closed
Closed
This was referenced Mar 16, 2021
Merged
Closed
@ChrisBAshton ChrisBAshton mentioned this pull request Mar 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@36degrees 36degrees 36degrees left review comments

@AlanGabbianelli AlanGabbianelli AlanGabbianelli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@richardTowers @36degrees @AlanGabbianelli