Responsive accessible table for API Users#index #2341
Merged
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
I assume that this name was helpful at the time it was chosen but I'm not sure it is anymore. "Component" means a lot of different things and right now within the Signon codebase I think that we're converging on it referring to GOV.UK Publishing Components and its ecosystem
And make some small changes to turn it into an app-based component.
We're trying to make our tables more usable on small screens.
Since this goal doesn't appear to contradict the existing Table
component's goals, we'd like to merge those improvements into the gem.
But first of all, we need to develop and test the changes, so we're
planning to:
1. ~~duplicate the gem's version of the component~~
2. make the changes locally
3. test them in our app, probably in production
4. open a pull request to merge the changes into the gem
5. switch the app back to using the gem's version
It's unclear what the cleanest strategy is to achieve this copying and
modifying. For instance, I've included CSS and JavaScript here although
we might have got away with just depending on the gem's versions? And
a hundred other dilemmas.
Changes I've already made to the copied files (to make the app not crash
and to turn this into an app-based component):
- change helper name and namespacing, and make class a module -- the
result is an unpleasant compromise between the gem's approach and
how I'd expect the app to do this if starting from scratch
- remove the `add_gem_component_stylesheet` line from the partial and
don't replace it with `add_app_component_stylesheet` -- I'm not sure
what the app one is meant to do, because it didn't appear to do what
adding `@import "components/*";` to application.scss did
- rename 'gem-c-table' and related CSS classes and JavaScript
selectors to 'app-c-table', etc
- remove inline warning comment from helper -- although part of me
wants to add warning comments to all of these files that I've copied
- add 1 Rubocop and 1 yarn stylelint ignore comment -- it's existing
code and we want to keep it as close as possible to the original
Instead of a text argument, similar to how a lot of view helpers work. But I'm going to avoid offering any flexibility in the order of arguments (that's a common view helper thing when there's a choice between text argument or block, right?). We'll just ignore the value supplied for that first argument if there is a block
To improve usability, we're going to introduce an alternate table layout for small screens and in the process we're going to change the display styling for <tr>s and <td>s. According [Adam Fenwick's write up of his Responsive Accessible Table](https://www.afenwick.com/blog/2021/responsive-accessible-table/), this "means a screen reader is no longer aware these are still table elements" but with these HTML roles in place, "no matter what we do to these elements, a screen reader always recognises them as their default".
The resulting line-broken text isn't necessarily pretty, but this does solve the problem on the API Users page where long email addresses can result in the <table> being wider than its container (div.govuk-width-container). I'm not sure how widely useful this is and therefore how to handle this when we're ready to merge our other changes back into the components gem.
Borrowing liberally from [Adam Fenwick's Responsive Accessible Table example](https://www.afenwick.com/blog/2021/responsive-accessible-table/). On tablet-sized screens and smaller, it switches the layout of the table into a vertical list of cards, which we believe is more usable for all. I've strayed from the example in the choice of screen size, because the specific case that I was using to test this (API Users index page) seems usable as a regular table on screens a fair bit smaller than "desktop". If that's not true for other tables in the app, we can easily change the hard-coded media queries or even supply the breakpoint as an argument to the component. I've essentially appended the new styling to the end of the existing component CSS because those existing styles apply more generally to all govuk-tables (i.e. they didn't specify the app-c-table class or prefix). I think it'll make more sense to merge the styles together when we're ready to merge our changes into the gem.
Using the app's new version of the Table component, this lays the table's contents out vertically on screens tablet-sized and below. In the related controller test, I've switched to using a regexp instead of a string to match the contents of the table cells. The implementation of `vertical_on_small_screen` embeds some markup in each table cell, which would break these strict matchers.
mike29736
force-pushed
the
responsive-accessible-table
branch
from
b5cf2a5 to
f0a8f53
Compare
floehopper
approved these changes
Sep 5, 2023
yndajas
added a commit
that referenced
this pull request
Jun 27, 2024
In #2341, the table component from `govuk_publishing_components` was copied over and customised In #2664, we migrated the applications tables to use this. In the process, we lost the custom classes on the first two columns, which set the widths to 1/4 and 1/3 respectively. This was noted as "acceptable" in the PR body in exchange for improved consistency and other benefits from using the component However, since we have a custom version of the table component, it's very easy for us to support custom classes. This was in fact done in the same PR that copies the component into the Signon codebase, adding custom class support for the outer table element in 4f09fbc This adds support for custom classes in the headers, which is then used to restore the old classes and therefore consistent column widths between tables (irrespective of content width) In #2341, it was suggested that we might try to get the customisations merged upstream - it might be worth exploring that with both this and earlier changes as a separate piece of work #2341: #2341 #2664: #2664
yndajas
added a commit
that referenced
this pull request
Jun 27, 2024
Since #2341, we've had support for making tables use a vertical layout on smaller viewports, similar to how the summary list reflows on smaller viewports (the end result is something of a hybrid of table and summary list layouts) We already use this on the users screen. This feels like something that would be useful on the applications screen too I've added an optional `visually_hidden` flag on table head cells here, which when set to `true` will result in a class that removes padding from the header cell. The current actions column on the applications page has a visually hidden header, so this allows us to remove the padding from visually hidden header cells while retaining the content
yndajas
added a commit
that referenced
this pull request
Jun 27, 2024
Since #2341, we've had support for making tables use a vertical layout on smaller viewports, similar to how the summary list reflows on smaller viewports (the end result is something of a hybrid of table and summary list layouts) We already use this on the users screen. This feels like something that would be useful on the applications screen too I've added an optional `visually_hidden` flag on table head cells here, which when set to `true` will result in a class that removes padding from the header cell. The current actions column on the applications page has a visually hidden header, so this allows us to remove the padding from visually hidden header cells while retaining the content Per f0a8f53, we switch to regex-based matching for some tests. The vertical layout implementation adds some hidden text in table cells that would otherwise break these `assert_select`s
yndajas
added a commit
that referenced
this pull request
Jun 28, 2024
Since #2341, we've had support for making tables use a vertical layout on smaller viewports, similar to how the summary list reflows on smaller viewports (the end result is something of a hybrid of table and summary list layouts) We already use this on the users screen. This feels like something that would be useful on the applications screen too I've added an optional `visually_hidden` flag on table head cells here, which when set to `true` will result in a class that removes padding from the header cell. The current actions column on the applications page has a visually hidden header, so this allows us to remove the padding from visually hidden header cells while retaining the content Per f0a8f53, we switch to regex-based matching for some tests. The vertical layout implementation adds some hidden text in table cells that would otherwise break these `assert_select`s
yndajas
added a commit
that referenced
this pull request
Jul 3, 2024
Since #2341, we've had support for making tables use a vertical layout on smaller viewports, similar to how the summary list reflows on smaller viewports (the end result is something of a hybrid of table and summary list layouts) We already use this on the users screen. This feels like something that would be useful on the applications screen too I've added an optional `visually_hidden` flag on table head cells here, which when set to `true` will result in a class that removes padding from the header cell. The current actions column on the applications page has a visually hidden header, so this allows us to remove the padding from visually hidden header cells while retaining the content Per f0a8f53, we switch to regex-based matching for some tests. The vertical layout implementation adds some hidden text in table cells that would otherwise break these `assert_select`s
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.
https://trello.com/c/dggyhTVY/62-api-users-page-fix-and-iterate-how-this-page-is-displayed
On tablet screens and smaller, give the option to render a table in a way that I'm finding it hard to describe... kind of a list of cards. In the code I've referred to it as "vertical".
To achieve this, I've made a copy of the Table component (from the GOV.UK Publishing Components gem) in order to try these changes out in Signon before attempting to get them merged back into the gem.
Without the feature enabled:
With the feature enabled:
