Skip to content
This repository has been archived by the owner on Jun 20, 2022. It is now read-only.

Replace manuals accordion with gem accordion #1060

Merged
merged 4 commits into from
Feb 12, 2021

Conversation

owenatgov
Copy link
Contributor

@owenatgov owenatgov commented Feb 5, 2021

What

Replace the custom manuals accordion (collapsible collection) with the components gem accordion. This includes:

  • Moving the processing of the markup for individual manuals from the js to the document model
  • Updating the updates view
  • Removing evidence of the previous accordion implementation. This includes all of jasmine as there are now no js modules within this repo to test and therefore no point in including the test suite

Why

This is part of work within the govuk accessibility team to ensure that accordions are consistent across gov.uk. Manuals frontend has been using a bespoke accordion implementation which bolts on accordion functionality on top of govspeak content from manuals publisher. This creates risk because any updates to our accordion can't reliably permeate to manuals and could create hidden legacy code and accessibility issues. This change creates a relationship between manuals and our components gem, as is the case with our other frontend apps, to minimise risk and increase consistency between manuals and the rest of gov.uk.

The team are conscious that this is, in a sense, perpetuating the hack and the ideal would be to change the publishing workflow to account for accordion content, thus mitigating the need for any sort of preprocessing of the content item. We don't have the resource or knowledge in the team to action this, however an issue has been raised addressing this need.

Card

Pages tested against

Visual changes

Before

Screenshot 2021-02-05 at 14 55 31
Screenshot 2021-02-05 at 14 56 43

After

Screenshot 2021-02-12 at 10 41 46
Screenshot 2021-02-12 at 10 42 14

@bevanloon bevanloon temporarily deployed to manuals-fron-replace-ma-swbuxj February 5, 2021 15:04 Inactive
@owenatgov owenatgov force-pushed the Replace-manuals-accordion-with-gem-accordion branch from ea745fb to 2030c05 Compare February 8, 2021 10:26
@bevanloon bevanloon temporarily deployed to manuals-fron-replace-ma-wzksuz February 8, 2021 10:27 Inactive
@owenatgov owenatgov force-pushed the Replace-manuals-accordion-with-gem-accordion branch from 2030c05 to d0bc6bb Compare February 8, 2021 11:24
@bevanloon bevanloon temporarily deployed to manuals-fron-replace-ma-wzksuz February 8, 2021 11:25 Inactive
@owenatgov owenatgov force-pushed the Replace-manuals-accordion-with-gem-accordion branch from d0bc6bb to d4889b9 Compare February 8, 2021 11:33
@bevanloon bevanloon temporarily deployed to manuals-fron-replace-ma-wzksuz February 8, 2021 11:33 Inactive
@owenatgov owenatgov force-pushed the Replace-manuals-accordion-with-gem-accordion branch from d4889b9 to 2f6cbe7 Compare February 8, 2021 18:11
@bevanloon bevanloon temporarily deployed to manuals-fron-replace-ma-wzksuz February 8, 2021 18:11 Inactive
@owenatgov
Copy link
Contributor Author

Note on tests

End to end test are currently failing. This is because this PR introduced a test failure in the publishing-e2e-tests repo due to the significant change in markup. I've created a separate PR to handle this. As per the e2e contribution guidance, once this has been reviewed and approved I'll set this branch to test against my e2e branch, merge my changes, deploy and then merge my e2e PR.

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.

LGTM - but made some code quality suggestions below.

I'd also amend the last commit to include the context you provided on Slack around why you're removing Jasmine 👍

app/models/document.rb Outdated Show resolved Hide resolved
app/models/document.rb Outdated Show resolved Hide resolved
app/models/document.rb Outdated Show resolved Hide resolved
app/views/manuals/_manual_section.html.erb Outdated Show resolved Hide resolved
app/views/manuals/_manual_section.html.erb Outdated Show resolved Hide resolved
app/views/manuals/_manual_section.html.erb Outdated Show resolved Hide resolved
app/views/manuals/_manual_updates.html.erb Show resolved Hide resolved
@owenatgov owenatgov force-pushed the Replace-manuals-accordion-with-gem-accordion branch from 2f6cbe7 to 2b9062e Compare February 12, 2021 10:39
@bevanloon bevanloon temporarily deployed to manuals-fron-replace-ma-wzksuz February 12, 2021 10:39 Inactive
@owenatgov owenatgov force-pushed the Replace-manuals-accordion-with-gem-accordion branch from 2b9062e to d1cbc7e Compare February 12, 2021 10:47
@bevanloon bevanloon temporarily deployed to manuals-fron-replace-ma-wzksuz February 12, 2021 10:47 Inactive
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.

LGTM 👍 thanks for making the changes! One or two non-blocking comments below:

app/models/document.rb Outdated Show resolved Hide resolved
app/models/document.rb Outdated Show resolved Hide resolved
@owenatgov owenatgov force-pushed the Replace-manuals-accordion-with-gem-accordion branch from d1cbc7e to b9b5f54 Compare February 12, 2021 11:09
@bevanloon bevanloon temporarily deployed to manuals-fron-replace-ma-wzksuz February 12, 2021 11:10 Inactive
As this is now using the gem accordion where there are already jasmine tests, any tests here would just be replicating effort. There's now no point in jasmine being part of this repo as there aren't any js modules to test.
@owenatgov owenatgov force-pushed the Replace-manuals-accordion-with-gem-accordion branch from b9b5f54 to d52faee Compare February 12, 2021 11:14
@bevanloon bevanloon temporarily deployed to manuals-fron-replace-ma-wzksuz February 12, 2021 11:14 Inactive
@owenatgov owenatgov merged commit 12e95be into master Feb 12, 2021
@owenatgov owenatgov deleted the Replace-manuals-accordion-with-gem-accordion branch February 12, 2021 11:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants