World Location News Article front-end #228

Merged
merged 12 commits into from Jan 17, 2017

Projects

None yet

4 participants

@bevanloon
Contributor

Tests will fail until the corresponding schema changes have been merged and deployed:
alphagov/govuk-content-schemas#476

Trello card

Add presenter, view and related artefacts needed to support rendering World Location News Articles in government-frontend.

Feature Old New
Simple example whitehall_basic government-frontent_basic
With attachments whitehall_attachments government-frontend_attachments
History mode whitehall_history_mode government-frontend_history_mode
Images in body whitehall_images government-frontend_images
Multiple translations whitehall_multiple_translations dev_multilpe_translations
Translated whitehall_world_location_news_article_translated government-frontend_world_location_news_article_translated
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-228 Jan 6, 2017 Inactive
@bevanloon bevanloon referenced this pull request in alphagov/govuk-content-schemas Jan 6, 2017
Merged

World Location News Article content schema and examples #476

+ translations: @content_item.available_translations
+ %>
+ </div>
+</div>
@fofr
fofr Jan 6, 2017 Member

This block should use the title_and_translations partial along with the TitleAndContext module in the presenter.

@bevanloon
bevanloon Jan 9, 2017 Contributor

refactored in 880736c

+ see_updates_link: true
+ %>
+ </div>
+</div>
@fofr
fofr Jan 6, 2017 Member

This block should use the Metadata module and partial

@bevanloon
bevanloon Jan 9, 2017 Contributor

refactored in 880736c

+ <div class="column-two-thirds offset-one-third">
+ <%= render 'shared/share_buttons', share_urls: @content_item.share_urls %>
+ </div>
+</div>
@fofr
fofr Jan 6, 2017 Member

The grid-row wrappers for share buttons could be moved into the partial. All uses of the partial have the same wrapper.

@bevanloon
bevanloon Jan 9, 2017 Contributor

👍 60f19b0 should do that

+ published: @content_item.published,
+ part_of: @content_item.part_of,
+ direction: page_text_direction
+%>
@fofr
fofr Jan 6, 2017 Member

This block should use the Metadata module and @content_item.document_footer to pass in parameters

@bevanloon
bevanloon Jan 9, 2017 Contributor

refactored in 880736c

config/locales/en.yml
@@ -225,6 +225,9 @@ en:
transparency:
one: Transparency data
other: Transparency data
+ world_location_news_article:
@fofr
fofr Jan 6, 2017 Member

document_type is always "news_article" in the schema examples. Is this string needed?
If we do need to keep it then we need to generate the translations for the other locales too, as this format can have translations.

@bevanloon
bevanloon Jan 9, 2017 Contributor

so this is currently used when rendering the history notice (https://github.com/alphagov/government-frontend/blob/master/app/views/shared/_history_notice.html.erb#L3).

I notice that 'news_article' is already translated - so do we want to fall back to that for our translation? Otherwise this probably needs to be translated

@bevanloon
bevanloon Jan 9, 2017 Contributor

Had a quick chat in the core channel about this and one suggestion from @boffbowsh is that eventually WLNA might be consolidated into being a "news_article". This sounds reasonable but I'm not sure what's involved in doing this though. It possibly needs some investigation (created trello card for this: https://trello.com/c/kAJM8OfE/815-investigate-consolidating-wlna-and-news-article) and if we do want to do that then perhaps it warrants a story of its own.

For now, I've kept the entry in en.yml so that history notices show for WLNA. In addition, I've added empty entries to the other locales for world_location_news_article with commit 8ec75a8 so it's consistent with the rest of the application - there are other empty entries in the locales (thanks @gpeng) as well so following prior art in the meantime.

@fofr
fofr Jan 10, 2017 Member

I think the history notice reads better as "This news article...". I don't think the user needs to understand the difference between news article and world location news article.

@Rosa-Fox
Rosa-Fox Jan 12, 2017 Contributor

Hi,

@fofr we can chat about this in the morning, but I have added a commit which translates 'world location news article' to 'news article' in English.

Reason for this was that I was going to pull in the document_type as opposed to the format in _history_notice which would mean it would pull in "document_type": "news_article" for WLNAs. Sadly this messed up the text for a few other formats, for example in Detailed Guides the text “This guidance” changed to “This detailed guide".

Could have added a condition to the view that pulled in the document_type for WLNAs and the format for the other doc_types but thought it might be better to keep it consistent and keep the blank lines in the translation files until we get a translation at some point.

@bevanloon
bevanloon Jan 13, 2017 Contributor

@Rosa-Fox - what did content think about simply leaving out the format from the history notice? That would mean everything would simply say "This was published under the..."?

Instead of a condition in the view, we could also use Rails' default option in the I18n.t - so we could have something like

I18n.t(
       "content_item.format.#{content_item.format}",
       default: ["content_item.format.#{content_item.document_type}".to_sym],
       count: 1
      ).downcase

Then if there were no translations for "World location news article" it would fall back to using document_type which would be "news article". My only real concern here is that it isn't really a default, it's a fallback so the naming is annoying.

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-228 Jan 9, 2017 Inactive
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-228 Jan 12, 2017 Inactive
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-228 Jan 16, 2017 Inactive
config/locales/en.yml
@@ -226,8 +226,8 @@ en:
one: Transparency data
other: Transparency data
world_location_news_article:
@fofr
fofr Jan 16, 2017 Member

We can remove this completely now can't we? And from all the translation files.
I believe the history partial was the only thing using it.

bevanloon and others added some commits Jan 9, 2017
@bevanloon @Rosa-Fox bevanloon Move grid-row wrappers for share buttons into partial 00f41fe
@Rosa-Fox Rosa-Fox Text change in history notice
- Not necessary to display document format to user in history notice.
- More concise.
6bf8980
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-228 Jan 16, 2017 Inactive
@Rosa-Fox Rosa-Fox Remove translation for world location news article
- Remove translation as no longer used
4b774b6
@Rosa-Fox
Contributor

@fofr have removed the world location news article translations.

@fofr fofr changed the title from [DO NOT MERGE] World Location News Article front end to World Location News Article front-end Jan 16, 2017
@fofr
fofr approved these changes Jan 16, 2017 View changes
@Rosa-Fox Rosa-Fox merged commit b5f1593 into master Jan 17, 2017

1 check passed

continuous-integration/jenkins/branch This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment