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.

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

Update World Location News and Topical Events templates #6147

Merged
merged 5 commits into from
Oct 5, 2021

Conversation

chris-gds
Copy link
Contributor

@chris-gds chris-gds commented Jun 11, 2021

What

Update World location news template to use components / the Design System

Why

On-going a11y work, to swap components, align architecture and, in turn, improve accessibility.

Visuals

Note (local "after" screenshots have missing images due to local data - this won't be an exact replica)

Before After
www gov uk_world_france_news(iPhone 6_7_8) (2) www integration publishing service gov uk_world_france_news(iPhone 6_7_8) (1)
www gov uk_world_france_news (1) www integration publishing service gov uk_world_france_news

How to test

Only some pages have "statistics documents" one example can be found below:
/world/falkland-islands/news

Other example URLs:
/world/france/news
/world/new-zealand/news

Anything else

[DO NOT MERGE]
There are multiple files with /app/views/shared that require updating across all templates in use
eg: recently_updated appears on /world/uk-and-the-commonwealth

Swapping document_list and translation_nav proved problematic, this has been omitted from this PR to be raised separately.

There are known logical heading order issues on these pages. This should be addressed in the document_list swap

During this work I faced some local development issues, I've updated the README with some troubleshooting notes.

@chris-gds chris-gds force-pushed the update-world-location-to-gem branch 5 times, most recently from b203771 to 4a0c79b Compare July 12, 2021 13:16
@chris-gds chris-gds mentioned this pull request Jul 12, 2021
@chris-gds chris-gds force-pushed the update-world-location-to-gem branch 2 times, most recently from b2381a4 to c6e2372 Compare July 12, 2021 14:18
@chris-gds chris-gds changed the title Draft: Swapping page elements to gem Update World Location News template Jul 12, 2021
@chris-gds chris-gds marked this pull request as ready for review July 12, 2021 14:22
@chris-gds chris-gds marked this pull request as draft July 12, 2021 14:24
@chris-gds chris-gds force-pushed the update-world-location-to-gem branch 5 times, most recently from d780940 to bc78522 Compare July 14, 2021 09:09
@chris-gds chris-gds marked this pull request as ready for review July 14, 2021 09:42
@chris-gds chris-gds force-pushed the update-world-location-to-gem branch 3 times, most recently from c2f96fc to e988a87 Compare July 14, 2021 15:53
app/views/shared/_recently_updated.html.erb Outdated Show resolved Hide resolved
app/views/shared/_recently_updated.html.erb Outdated Show resolved Hide resolved
app/views/world_location_news/index.html.erb Outdated Show resolved Hide resolved
app/views/world_location_news/index.html.erb Outdated Show resolved Hide resolved
app/views/world_location_news/index.html.erb Outdated Show resolved Hide resolved
app/views/world_locations/show.html.erb Show resolved Hide resolved
test/unit/helpers/document_helper_test.rb Show resolved Hide resolved
@chris-gds chris-gds force-pushed the update-world-location-to-gem branch 2 times, most recently from 5deb838 to ffbd4b6 Compare July 21, 2021 15:18
@chris-gds chris-gds force-pushed the update-world-location-to-gem branch 3 times, most recently from 1fd5497 to 777b30f Compare July 21, 2021 16:36
@chris-gds chris-gds requested a review from owenatgov July 22, 2021 16:16
Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

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

A few comments. Glad to see this out the door finally!

app/views/topical_events/show.html.erb Outdated Show resolved Hide resolved
app/assets/stylesheets/frontend/views/_topical-events.scss Outdated Show resolved Hide resolved
app/views/topical_events/show.html.erb Outdated Show resolved Hide resolved
app/views/topical_events/show.html.erb Outdated Show resolved Hide resolved
app/views/topical_events/show.html.erb Outdated Show resolved Hide resolved
app/views/topical_events/show.html.erb Outdated Show resolved Hide resolved
@owenatgov owenatgov dismissed their stale review October 1, 2021 13:05

The PR has been added to so I'm withdrawing my approval until it's ready again

@jon-kirwan jon-kirwan force-pushed the update-world-location-to-gem branch 7 times, most recently from cf0fc29 to e549539 Compare October 1, 2021 17:48
Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

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

Some final comments. We're real close 👍

app/assets/stylesheets/frontend/views/_topical-events.scss Outdated Show resolved Hide resolved
app/views/topical_events/show.html.erb Outdated Show resolved Hide resolved
@jon-kirwan jon-kirwan force-pushed the update-world-location-to-gem branch 3 times, most recently from e2bb853 to f436862 Compare October 4, 2021 16:50
Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

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

🚀

Chris Yoong and others added 5 commits October 5, 2021 11:45
- Convert bespoke UI elements within "world-location" to use the component gem, applying Design System when appropriate including the layout. 

- Remove some redundant CSS
- Semi-maintain shared elements that appear elsewhere
- Include horizontal rule as part heading
- Update testing

Eg page:
/world/organisations/british-embassy-moscow/about/recruitment
/world/falkland-islands/news

"document_list" & "translation_nav" swaps omitted (from this PR)
Altering shared files that generate document lists to align with an updated design
Remove top margin
Improving nested headings within document lists
@jon-kirwan
Copy link
Contributor

jon-kirwan commented Oct 5, 2021

🚀

Cheers Owen! Could I ask you to hit the merge button for me too 👍 (I've rebased the branch just now)

@owenatgov owenatgov merged commit 8360b6c into main Oct 5, 2021
@owenatgov owenatgov deleted the update-world-location-to-gem branch October 5, 2021 11:11
@owenatgov owenatgov changed the title [DO NOT MERGE] Update World Location News and Topical Events templates Update World Location News and Topical Events templates Oct 5, 2021
@chris-gds chris-gds restored the update-world-location-to-gem branch October 8, 2021 14:16
chris-gds pushed a commit that referenced this pull request Oct 8, 2021
Temporarily reverting as merge impacts "http://www.gov.uk/world/uk-and-the-commonwealth"
chris-gds pushed a commit that referenced this pull request Oct 8, 2021
Temporarily reverting as merge impacts "http://www.gov.uk/world/uk-and-the-commonwealth"
This was referenced Oct 8, 2021
@barrucadu barrucadu deleted the update-world-location-to-gem branch November 24, 2021 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants