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

Add rendering of Field of operation pages #2736

Merged
merged 3 commits into from
Apr 12, 2023
Merged

Add rendering of Field of operation pages #2736

merged 3 commits into from
Apr 12, 2023

Conversation

JonathanHallam
Copy link
Contributor

This is part of ongoing work to stop Whitehall from rendering anything

Whitehall

Screenshot 2023-04-05 at 11 15 46

Government Frontend

Screenshot 2023-04-05 at 11 19 51

Trello - https://trello.com/c/alzDKosv/472-add-rendering-of-field-of-operation-pages-to-government-frontend

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2736 April 5, 2023 10:20 Inactive
BeckaL
BeckaL reviewed Apr 5, 2023
View reviewed changes
app/presenters/field_of_operation_presenter.rb
class FieldOfOperationPresenter < ContentItemPresenter
include ContentItem::TitleAndContext

FatalityNotice = Struct.new(:roll_call_introduction, :casualties, :title, :base_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice :-)

BeckaL
BeckaL reviewed Apr 5, 2023
View reviewed changes
Copy link
Contributor

@BeckaL BeckaL left a comment

Choose a reason for hiding this comment

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

Nice, looks generally good to me, a few suggestions / comments below

config/locales/en.yml Outdated Show resolved Hide resolved
app/views/content_items/field_of_operation.html.erb Outdated Show resolved Hide resolved
app/views/content_items/field_of_operation.html.erb Show resolved Hide resolved
config/locales/ar.yml Show resolved Hide resolved
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2736 April 5, 2023 15:54 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2736 April 5, 2023 15:55 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2736 April 6, 2023 07:53 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2736 April 6, 2023 08:01 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2736 April 6, 2023 08:08 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2736 April 6, 2023 08:19 Inactive
@BeckaL
Copy link
Contributor

BeckaL commented Apr 6, 2023

Nice, this is looking really good. Noticed that the logo now renders incorrectly, I wonder if we can specify that we want to use the organisation title rather than the 'formatted title' from the content item
Screenshot 2023-04-06 at 10 30 07

@JonathanHallam
Copy link
Contributor Author

Nice, this is looking really good. Noticed that the logo now renders incorrectly, I wonder if we can specify that we want to use the organisation title rather than the 'formatted title' from the content item Screenshot 2023-04-06 at 10 30 07

We can, but we'd lose the break which we should have, really should have spotted this, sorry, looking for a way I can do both the html_safe / sanitize thing that whitehall is currently doing

@JonathanHallam
Add rendering of field of operation pages
9d40e56 
This is part of some longstanding work to remove rendering from whitehall

It required me to add some i18n, unfortunately running the task to add missing locales also fixed some speech marks that were off the wrong type for the generation. Sorry for the extra noise!
@JonathanHallam
Add none added i18n from whitehall
e782200 
This adds the i18n from whitehall that's used if a field of operation page has no attached fatlity notice.
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2736 April 6, 2023 09:42 Inactive
@JonathanHallam
Copy link
Contributor Author

@BeckaL have fixed this with html_safe

BeckaL
BeckaL approved these changes Apr 6, 2023
View reviewed changes
Copy link
Contributor

@BeckaL BeckaL left a comment

Choose a reason for hiding this comment

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

Fab, looks good to me, happy for this to go to F&V

catalinailie
catalinailie approved these changes Apr 12, 2023
View reviewed changes
Copy link
Contributor

@catalinailie catalinailie left a comment

Choose a reason for hiding this comment

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

Looks good to me ⭐

@chao-xian
Copy link
Contributor

@JonathanHallam which is the example page from https://government-frontend-pr-2736.herokuapp.com/ ?

@JonathanHallam
Copy link
Contributor Author

@chao-xian https://government-frontend-pr-2736.herokuapp.com/government/fields-of-operation/iraq

Which reminds me that there's the examples in the readme for this app, I'll add a commit to add it there

@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2736 April 12, 2023 11:31 Inactive
@JonathanHallam JonathanHallam merged commit 5083c46 into main Apr 12, 2023
@JonathanHallam JonathanHallam deleted the field-of-operation branch April 12, 2023 13:38
@JonathanHallam JonathanHallam mentioned this pull request Apr 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@catalinailie catalinailie catalinailie approved these changes

@BeckaL BeckaL BeckaL approved these changes

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
@JonathanHallam @BeckaL @chao-xian @catalinailie @govuk-ci