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

Create basic page for Worldwide Organisations in government-frontend #2616

Merged
merged 2 commits into from
Dec 28, 2022

Conversation

callumknights
Copy link
Contributor

@callumknights callumknights commented Dec 1, 2022

Create initial basic page for Worldwide Organisations in government-frontend.

The page currently only consists of the title and content item description, more parts of the page can be added later when more is presented to the content store by whitehall, metadata, page body etc.

Also included basic testing which can be built upon later as the page is migrated.

The breadcrumb trail at the top of the page wasn't previously in the whitehall rendering, but I feel it adds something to the page to show that, happy to be corrected though.

image

New page running locally ⬆️

Old page on whitehall (with full content) ⬇️

image

Trello: https://trello.com/c/uiw7XFjE/379-create-routes-controllers-and-views-for-worldwide-organisation-pages-in-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-2616 December 1, 2022 15:45 Inactive
@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from b950a71 to 4bba41c Compare December 12, 2022 15:50
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 12, 2022 15:51 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 12, 2022 15:59 Inactive
@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from 6d8ec64 to 3b7968b Compare December 12, 2022 16:35
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 12, 2022 16:36 Inactive
@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from 3b7968b to d3c06da Compare December 12, 2022 16:41
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 12, 2022 16:41 Inactive
@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from d3c06da to efbaf9a Compare December 13, 2022 16:47
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 13, 2022 16:47 Inactive
@callumknights callumknights marked this pull request as ready for review December 13, 2022 17:08
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.

Great to see the work on worldwide organisations started ⭐ !

Although this is just the barebones stuff, there are some differences here between the title and description as rendered by this PR and the currently live one. If we've explicitly decided to change how the page is rendered, could you note that in this PR. If not, I think worth doing the bit of work now to get them looking the same, to save us extra PRs later.

Once you've done that, worth having a good review of the code that's in here and what's actually needed - I think a few things have slipped in via copy and paste that aren't required (some noted below, possibly also the translations bit of the view - are worldwide organisations ever actually translated? I'm not sure).

If you could also include screenshots in your PR of the live page as a comparison, that'd be useful.

app/views/content_items/worldwide_organisation.html.erb Outdated Show resolved Hide resolved
app/presenters/worldwide_organisation_presenter.rb Outdated Show resolved Hide resolved
Copy link
Member

@brucebolt brucebolt left a comment

Choose a reason for hiding this comment

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

This is looking good. I notice there's a few visual differences between the heading on your screenshot and the live page. It'd be good if we could get this looking the same now, so we don't have to correct these later on.

@@ -417,3 +417,18 @@ ja:
working_group:
contact_details: 連絡先の詳細
policies: 方針
worldwide_organisation:
Copy link
Member

Choose a reason for hiding this comment

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

Looking in Whitehall, it appears we have translation for most languages, e.g. Japanese.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've hopefully corrected the visual differences that need doing, but given the final design of this page is yet to be decided, I'm not sure how much more time its worth putting in making it perfect now. I'll follow this up with a commit for the translation content, as it will be a copy paste marathon, and I'd like to get opinions on the other changes while doing that.

Copy link
Member

@brucebolt brucebolt Dec 15, 2022

Choose a reason for hiding this comment

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

It's very hacky, but I've adapted my code for moving locale entries from one app to another for Government Frontend, so you won't need to do any manual copy and pasting:

cd ~/govuk/whitehall
govuk-docker-run rake "translation:export:all[locale_export]"

cd locale_export
mkdir updated

for f in *.csv                               
do
  grep 'key,source,translation\|worldwide_organisation' $f > updated/$f              
done

cd ~/govuk/government-frontend
cp -r ~/govuk/whitehall/locale_export/updated ./

govuk-docker-run rake "translation:export:all[locale_export]"

cd locale_export
mkdir updated

for f in *.csv                               
do
  cat $f > updated/$f
  grep -v 'key,source,translation' ../updated/$f >> updated/$f
done

cd ..

govuk-docker-run rake "translation:import:all[locale_export/updated]"
govuk-docker-run rake "translation:normalize"

If you get any unexpected changes (e.g. unrelated keys being moved into alphabetical order), run govuk-docker-run rake "translation:normalize" in government-frontend before all this, then commit the changes as a separate commit which normalizes the locale files.

@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from efbaf9a to c019232 Compare December 14, 2022 09:26
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 14, 2022 09:26 Inactive
@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from c019232 to 459bdee Compare December 14, 2022 09:30
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 14, 2022 09:30 Inactive
@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from 459bdee to d01368c Compare December 15, 2022 14:45
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 15, 2022 14:46 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 15, 2022 14:59 Inactive
@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from ecc0269 to dbf9429 Compare December 15, 2022 15:06
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 15, 2022 15:07 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 15, 2022 16:49 Inactive
@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from 3594e31 to dbf9429 Compare December 15, 2022 16:52
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 15, 2022 16:52 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 15, 2022 17:05 Inactive
@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from f5191ac to 900fbe6 Compare December 15, 2022 17:24
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 15, 2022 17:25 Inactive
@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from 900fbe6 to dbf9429 Compare December 15, 2022 17:25
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 15, 2022 17:26 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 15, 2022 17:29 Inactive
@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from bba189f to dbf9429 Compare December 15, 2022 17:30
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 15, 2022 17:30 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 15, 2022 17:53 Inactive
@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from b4f4936 to 85f2802 Compare December 16, 2022 10:06
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 16, 2022 10:06 Inactive
@callumknights callumknights force-pushed the add-worldwide-organisations-basic-page branch from 85f2802 to 27d3ba7 Compare December 16, 2022 10:31
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-2616 December 16, 2022 10:31 Inactive
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.

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

config/locales/ar.yml Outdated Show resolved Hide resolved
@callumknights callumknights merged commit caf8d4f into main Dec 28, 2022
@callumknights callumknights deleted the add-worldwide-organisations-basic-page branch December 28, 2022 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants