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

Link 'Trade Tariff' to its website #2687

Merged
merged 1 commit into from
Aug 17, 2016
Merged

Link 'Trade Tariff' to its website #2687

merged 1 commit into from
Aug 17, 2016

Conversation

selfthinker
Copy link
Contributor

@selfthinker selfthinker commented Aug 12, 2016

Trello card

Description

This implements the suggested change in #2492, i.e. it links the words 'Trade Tariff' in the additional-commodity-code outcome to its respective website (https://www.gov.uk/trade-tariff).

The question artefacts correctly only contain that copy change.

Fact check

Expected changes

E.g. /additional-commodity-code/y/5/70/0/6 includes the link. This affects any additional-commodity-code outcome if there is a commodity code.

screen shot 2016-08-12 at 14 18 04

@selfthinker selfthinker force-pushed the link-trade-tariff branch 2 times, most recently from 2ecd8f8 to 15d35b5 Compare August 12, 2016 13:59
@pmanrubia pmanrubia added the needs content review Waiting for a content designer to approve label Aug 12, 2016
@pmanrubia pmanrubia self-assigned this Aug 12, 2016
@pmanrubia
Copy link
Contributor

pmanrubia commented Aug 12, 2016

Thank you for your work on it.

Two minor comments:

  1. We would need to fact-check this change with the department, so we need to create a Heroku app as described in this document.
  2. Could you please update the description following a similar structure than Denmark marriage tool update - Content change request #2653, or Same sex marriage in Italy #2688. All our PRs for smart-answers follow a similar approach. It simplifies our deployment process.

I have just checked that Github is struggling to show the commit because we have too many artefacts. In general, it is better to follow the approach you have chosen for this PR because we have atomic PRs, but in this situations, we sometimes split the commit into a) commit with the changes, b) commit with artefacts. I am not suggesting to do it for this PR; it is absolutely fine what you have done it.

@selfthinker
Copy link
Contributor Author

@pmanrubia, done both. Is the description okay this way?

@pmanrubia
Copy link
Contributor

LGTM 👍

@pmanrubia
Copy link
Contributor

@selfthinker I last thing..., could you please add a note to the card so we close the issue once it is deployed

@selfthinker
Copy link
Contributor Author

The issue will automatically be closed by GitHub when this is merged into master (because I added closes #2492 to the commit message). But we should update the ticket manually anyway as it is more polite.

Link 'Trade Tariff' in additional-commodity-code outcome
to its respective website (https://www.gov.uk/trade-tariff).

The question artefacts correctly only contain that copy change.
@selfthinker selfthinker removed the needs content review Waiting for a content designer to approve label Aug 17, 2016
@selfthinker selfthinker merged commit d86a86d into master Aug 17, 2016
@selfthinker selfthinker deleted the link-trade-tariff branch August 17, 2016 13:15
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.

2 participants