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

Update get involved #2284

Merged
merged 2 commits into from
Nov 17, 2021
Merged

Update get involved #2284

merged 2 commits into from
Nov 17, 2021

Conversation

chris-gds
Copy link

@chris-gds chris-gds commented Nov 15, 2021 •
edited
Loading

What

Update get involved page with redesign

Why

Apply publishing gem components + Design System to align architecture, improve consistency and in turn improve accessibility

Visuals

Provided Design Redesigned_Get_involved
Mobile Desktop
government-frontend dev gov uk_government_get-involved(iPhone 6_7_8) (1) government-frontend dev gov uk_government_get-involved (1)

Anything else

  • The "blue pull out" on provided design doesn't exist - this has been replaced with inset component
     as this component is most suitable based on context and properties
  • Lead paragraph text maintains inline anchor links from original page.
  • "Some active government blogs include" have a selection of links that appear to be a type of "document list" this was not appropriate so was replaced for a link list
  • "Take Part" in the original design has introduction copy, this doesn't exist and has been removed.
  • Two images in the image grid are missing alt copy "National Service" "Help Making your neighbour.." these have been requested to be updated via Content.
  • The original design omits the image grid, this has been kept at column-full-width inline with other pages.
  • The image grid uses a solution that is related to an layout feature request on the design system
  • The image grid section has been updated to be more inline with other instances

Render page locally

content-store, search-api, static have to be run in parallel
government-frontend has to be started in -app mode not app-live mode as local data is needed

Recent local data is needed for content-store and search-api - this data has to be pulled from AWS so its gets recent snapshot - eg:
gds aws govuk-integration-poweruser ./bin/replicate-elasticsearch.sh
gds aws govuk-integration-poweruser ./bin/replicate-mongodb.sh content-store

I faced an issue where data from search-api was failing to download correctly from AWS.

The workaround is to delete the parent folder govuk-docker/replication/elasticsearch-6
Once this is done each app has to be re made using make (as the existing containers will still reference old data)

@govuk-ci govuk-ci temporarily deployed to government-f-update-get-cp2m8n November 15, 2021 11:11 Inactive
@chris-gds chris-gds marked this pull request as ready for review November 15, 2021 11:23
@jon-kirwan
Copy link
Contributor

👍 Looking good to me. I've just mentioned a couple of minor things and added one or two questions / suggestions.

@govuk-ci govuk-ci temporarily deployed to government-f-update-get-cp2m8n November 15, 2021 20:12 Inactive
@chris-gds chris-gds requested a review from jon-kirwan November 15, 2021 20:17
jon-kirwan
jon-kirwan approved these changes Nov 16, 2021
View reviewed changes
Copy link
Contributor

@jon-kirwan jon-kirwan left a comment

Choose a reason for hiding this comment

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

Yep great! Just one small comment but otherwise all fine so have approved.

@govuk-ci govuk-ci temporarily deployed to government-f-update-get-cp2m8n November 16, 2021 11:00 Inactive
Chris Yoong added 2 commits November 16, 2021 11:04
Change "get-involved" Page
58e6783 
- Update Design of this page

- Use publishing_gem components and Design System and in turn improve consistency and on-going Accessibility 

- Adding inset-text component for use with pull out section that highlights to users a closing consultation

- Removing bespoke "get-involved" CSS

- Change load order of CSS.

The number within "Big number" component was pulling through an unexpected underline from 'govuk-link' on inspection this is coming through from 'component_support' - in this context it's due to the cascade, by moving the load order this resolve this issue as "big numbers" is being loaded after resolving the issue.

- Adjust layout for "take part" section that uses image_card component grid. As this is a list this uses an adjusted technique while a layout feature request on the Design System is outstanding,  This allows the Design system grid to be used but AT to interpret the section as a list [1]

[1] alphagov/govuk-frontend#2328
Move travel advice styles
94270a2 
- Remove bespoke "get involved" CSS from Application, this is not needed as all styling is coming from the gem and Design System

- There appeared to be some "travel-advice" styling in the top-level application.css - this isn't the right place for this, so this has been moved to travel-advice.css
@govuk-ci govuk-ci temporarily deployed to government-f-update-get-cp2m8n November 16, 2021 11:05 Inactive
@chris-gds chris-gds mentioned this pull request Nov 16, 2021
Open
@maxgds
Copy link
Contributor

maxgds commented Nov 16, 2021

I think the decision you've made is fine as is, but I think the blue box is an older iteration of the notice component:
https://components.publishing.service.gov.uk/component-guide/notice Let me know if you want to switch that in instead, otherwise I'll push this tomorrow.

@chris-gds
Copy link
Author

chris-gds commented Nov 17, 2021 •
edited
Loading

Hey @maxgds

I think the blue box is an older iteration of the notice component:
https://components.publishing.service.gov.uk/component-guide/notice Let me know if you want to switch that in instead, otherwise I'll push this tomorrow.

So, I considered this, however the when I was looking at the Design System guidance:

Use a notification banner to tell the user about something they need to know about, but that’s not directly related to the page content.

I felt this was directly related to the page in this context. I didn't quite think this was quite suitable even though visually it appeared to be close to the Design.

Also the guidance says

If the information is directly relevant to the thing the user is doing on that page, put the information in the main page content instead. Use inset text or warning text if it needs to stand out.

The warning text didn't seem suitable however, looking at the inset guidance (below) this felt more apt in this context

Use the inset text component to differentiate a block of text from the content that surrounds it, for example:
quotes
examples
additional information about the page

@maxgds
Copy link
Contributor

maxgds commented Nov 17, 2021

I think the page has so much going on you could make the argument that going to find out about a closing thing is sufficiently different to merit it - but I can see your argument and I'm not going to push for the change. I'll merge now.

@maxgds maxgds merged commit a98b84c into main Nov 17, 2021
@maxgds maxgds deleted the update-get-involved branch November 17, 2021 11:17
@brucebolt
Copy link
Member

@chris-gds It appears these changes have caused smokey to fail (see the latest run). Are you able to apply a fix to the smokey tests?

(Also paging @Tetrino who added the tests and may be able to give some more context)

@chris-gds
Copy link
Author

Hey @brucebolt thanks (I don't have access to prod) currently reviewing the smokey repo. Will liaise w/ @Tetrino

@chris-gds
Copy link
Author

Related smokey testing update: alphagov/smokey#871

@chris-gds chris-gds mentioned this pull request Nov 18, 2021
Merged
brucebolt added a commit to alphagov/smokey that referenced this pull request Nov 23, 2021
@brucebolt
Move 'Get Involved' tests to government-frontend app tests
33062ac 
When run through the continuous deployment pipeline, only the tests for
the specified app are run. When government-frontend has been deployed
recently, the 'Get Involved' tests never ran. As the change in [1]
caused these tests to fail but the tests never ran in CD, the change was
deployed to production and we only noticed when the smokey loop failed
in production.

Moving these tests to the government-frontend app will ensure they run
when after government-frontend is deployed to each environment.

1: alphagov/government-frontend#2284
@brucebolt brucebolt mentioned this pull request Nov 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jon-kirwan jon-kirwan jon-kirwan approved these changes

@maxgds maxgds Awaiting requested review from maxgds

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

Successfully merging this pull request may close these issues.
5 participants
@chris-gds @jon-kirwan @maxgds @brucebolt @govuk-ci