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

Remove translation nav component #886

Merged
merged 3 commits into from
May 11, 2018
Merged

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented Apr 30, 2018

The translation nav component has been moved from this app to the publishing components gem. This PR removes it and updates government-frontend to use the component from the gem.


Trello card: https://trello.com/c/ehg2nkE9/53-modify-component-translation-nav

Visual regression results:
https://government-frontend-pr-886.surge.sh/gallery.html

Component guide for this PR:
https://government-frontend-pr-886.herokuapp.com/component-guide/translation-nav

@andysellick andysellick changed the title Remove translation nav component [DO NOT MERGE] Remove translation nav component Apr 30, 2018
@tijmenb tijmenb temporarily deployed to government-frontend-pr-886 April 30, 2018 10:54 Inactive
@andysellick andysellick force-pushed the remove-translation-nav-component branch from bb80d8d to 54c54d9 Compare May 1, 2018 15:25
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-886 May 1, 2018 15:25 Inactive
@andysellick andysellick requested a review from vanitabarrett May 2, 2018 08:01
@andysellick andysellick changed the title [DO NOT MERGE] Remove translation nav component Remove translation nav component May 2, 2018
@sihugh
Copy link
Contributor

sihugh commented May 3, 2018

Added to govuk_publishing_components in alphagov/govuk_publishing_components#289

@sihugh sihugh self-assigned this May 3, 2018
Copy link
Contributor

@sihugh sihugh left a comment

Choose a reason for hiding this comment

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

I've checked the following pages locally (they have translations) and they use the component and look fine:

I've checked that the things that have been removed
are those that have been added to the govuk_publishing_components gem.

If we could get the detailed guide view to use the partial as the others do, then I think we're good to go.

@@ -4,7 +4,7 @@
</div>
<% if @content_item.available_translations.length > 1 %>
<div class="column-third">
<%= render 'components/translation-nav',
<%= render 'govuk_publishing_components/components/translation-nav',
Copy link
Contributor

Choose a reason for hiding this comment

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

If you could pull this section out and call the translations partial (below) like the other views do that'd be good.
<%= render 'shared/translations' %>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot @sihugh thanks, have updated.

@sihugh sihugh merged commit 8eeb0c4 into master May 11, 2018
@sihugh sihugh deleted the remove-translation-nav-component branch May 11, 2018 08:47
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