-
Notifications
You must be signed in to change notification settings - Fork 17
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
Reflect travel advice alert status from travel advice publisher on the advice page #2922
Conversation
e129550
to
301dbaa
Compare
301dbaa
to
f00c0ab
Compare
f00c0ab
to
2eb9d85
Compare
2eb9d85
to
ec7aedb
Compare
ec7aedb
to
6b878b9
Compare
6b878b9
to
a4eb048
Compare
a4eb048
to
e7f8039
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description is wonderful! Thanks for explaining the reasoning behind all of the decisions taken.
I noticed that the presenter tests were failing, so I had a look. I don't think that the country name was being passed in to the translation file as expected. Perhaps the %
makes it work on the page?
I thought it might be beneficial to check that the country name is being presented in the alert status correctly. However when I tried to test that the presenter was returning the country name, the test kept bombing.
It's an easy fix though. In the presenter, change the alert_status
method to pass in the country:
alert_statuses.map do |alert|
if allowed_statuses.include?(alert)
I18n.t("travel_advice.alert_status.#{alert}_html", country: country_name).html_safe
end
end
And then update the presenter test to check for the country name:
test "presents alert statuses" do
example = schema_item("full-country")
example["details"]["alert_status"] = %w[avoid_all_but_essential_travel_to_parts avoid_all_travel_to_parts]
presented = present_example(example)
assert presented.alert_status.first.include?("advise against all but essential travel to parts of #{presented.country_name}")
assert presented.alert_status.last.include?("advise against all travel to parts of #{presented.country_name}")
end
I'm happy to chat in person about this.
e7f8039
to
928f64d
Compare
Thanks for the review @leenagupte 🙌 I think I've addressed all your comments if you've got time to have a look. I wasn't able to run the test(s) locally so couldn't quite figure out what was wrong. Your example was really helpful - let me know if you'd like me to add you as a co-contributor to the commit 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove code from the view that would have rendered travel alerts as part of govspeak. To our knowledge this code has never been used as it wasn't possible to set alert_status in the editor until recently. Also remove formatting from the presenter which was customised for rendering the alerts in govspeak by flattening the array of alert statuses and wrapping each in a paragraph tag. This is in readiness for rendering the alerts with warning text in the next commit.
The alert status copy uses the abbreviation markup as recommended by the GOV.UK styleguide. Wrap the alerts in govspeak to apply the govspeak styling for acronyms.
928f64d
to
417bcfb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 🎉
I just wanted to add that as well as the PR description, the atomic commits and informative commit messages made this change really easy to review, so thank you! 🏅
2b4a565
to
6fef643
Compare
What
Publishers can now set an alert status for a country programmatically instead of including it in the copy content (see screenshot below for the editor options).
This PR:
alert_status
in the editor until recently. Removes formatting from the presenter which was customised for rendering the alerts in govspeak: it used to flatten the array of alert statuses and wrap them each in a paragraph tag. These changes are in readiness for rendering the alerts with warning text.Why
This will make it easier for publishers to update the alert status, keep it consistent in implementation, make it more visible for users and improve the semantics of the page.
Further information
As these alerts will replace the alerts previously set in the body copy, publishers will need to remove any alerts from the body copy when they set the status using the travel advice publisher checkboxes.
This change means that whilst travel alerts were previously rendered using the govspeak callout component, they will now be rendered using the warning text. The warning text contains visually hidden text to help screen reader users understand the content of the alerts.
Since the editor options were implemented as checkboxes, a publisher could set multiple statuses even though only one status will probably ever be set. The logic should therefore work with up to four options. We might change the options to radios in a further iteration.
Before
After
Fixes https://trello.com/c/Y4fruQto/1987-reflect-travel-advice-alert-status-from-travel-advice-publisher-on-the-advice-page-m
Follow these steps if you are doing a Rails upgrade.