Refactor applications tables #2664
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
chrislo
changed the title
chrislo
force-pushed
the
refactor-applications-tables
branch
3 times, most recently
from
f575348
to
25a29e4
Compare
I want to use this helper method in other application tables so I'm first moving it to a more generically-named helper.
I want to change this table to use the table component as we do in /app/views/api_users/applications/index.html.erb. This commit allows us to reuse the ApplicationTableHelper#update_permissions_link in both places to reduce duplication and logic in the views to make this transition easier to follow.
I want to change this table to use the table component as we do in /app/views/api_users/applications/index.html.erb. This commit allows us to reuse the ApplicationTableHelper#update_permissions_link in both places to reduce duplication and logic in the views to make this transition easier to follow.
Having this if/elsif branching logic in the template makes it harder to use the table component, so I'm extracting it into a helper method.
Having this if/elsif branching logic in the template makes it harder to use the table component, so I'm extracting it into a helper method. I've decided to use "real" user objects in the tests to avoid overly complex stubbing.
Having this logic in the template makes it harder to use the table component, so I'm extracting it into a helper method. I've decided to use "real" user objects in the tests to avoid overly complex stubbing.
Having this logic in the template makes it harder to use the table component, so I'm extracting it into a helper method.
I've decided to use "real" user objects here rather than perform complex stubbing of `policy` and `UserApplicationPermission.for`.
This will make it easier to use the table component in a consistent way in a subsequent commit.
This will make it easier to use the table component in a consistent way in a subsequent commit.
This helps remove some of the duplication in the markup and ensures that any changes that are made to the table component upstream are reflected in signon when we update the govuk_publishing_components gem. Note that the table component does not support setting the width of the columns or aligning the cells to the right (unless we use the "numeric" format argument in `rows` which doesn't seem appropriate semantically here). I'll address that in a subsequent commit.
This helps remove some of the duplication in the markup and ensures that any changes that are made to the table component upstream are reflected in signon when we update the govuk_publishing_components gem. Note that the table component does not support setting the width of the columns or aligning the cells to the right (unless we use the "numeric" format argument in `rows` which doesn't seem appropriate semantically here). I'll address that in a subsequent commit.
The table component renders the string "Not set" if a row is provided with `nil` for one of the `text` values[1]. This isn't very useful in the context of the columns of these tables that have a visually hidden header and contain "actions" (e.g. "Remove access"). Returning an empty string instead ensures that the cell is rendered blank in this case. [1] https://github.com/alphagov/govuk_publishing_components/blob/ba139b8a2e77a81c93484173101238ca1c40d001/spec/components/table_spec.rb#L47
The `govuk_publishing_components` `table` component does not support aligning columns (without resorting to setting the format to `numeric` which doesn't feel semantically correct). To work around this whitehall wraps these tables in a div with class `govuk-table--with-actions` and aligns the final cell in each row with a CSS override[1]. After discussion with Calum to determine that it is important to visually align these actions to the right, I've adopted something similar here. Since our final actions column can have a button-styled link as well as a text one, I've decided to align them to the right with a small gap between using a flex container. When this lands I'll raise the fact that this is a pattern that is now being used in a few places across `alphagov` repos and see if it's something that might be considered as a change to push up to `govuk_publishing_components`. That would allow us and whitehall to remove this potentially brittle CSS override. [1] https://github.com/search?q=org%3Aalphagov%20govuk-table--with-actions&type=code
chrislo
force-pushed
the
refactor-applications-tables
branch
from
25a29e4
to
901963c
Compare
chrislo
changed the title
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
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.
Trello: https://trello.com/c/yDM25WlZ
This PR removes some duplication of logic in the views that render the
/account/applications,/users/:id/applicationsand/api_users/:id/applicationspages. It also removes the hard-coded HTML and instead relies on thegovuk_publishing_componentstablecomponent to generate the markup.In the final commit I introduce a small CSS override modelled on a similar pattern used in Whitehall to align the final "actions" column of the table to the right.
There are some small visual differences after this refactor (notably we've lost the ability to specify the widths of the first two columns of the tables), but I've gone over these with Calum and we're happy that the changes introduced are acceptable given the overall improvement to consistency and benefits of using the table component.
/account/applications After
/account/applications Before