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

Content changes for Marriage Abroad (same sex civil partnerships in Greece) #2708

Merged
merged 1 commit into from
Sep 2, 2016

Conversation

@robinjam robinjam added needs content review Waiting for a content designer to approve Ready for code review labels Aug 26, 2016
@robinjam robinjam changed the title Same sex civil partnerships in greece Content changes for Marriage Abroad (same sex civil partnerships in Greece) Aug 26, 2016
Once the agreement has been signed, you’ll need to register the agreement with the local town hall. The registrar will then issue you with a special registration certificate.
<% end %>

##Naturalisation of your partner if they move to the UK
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a partial template (_partner_naturalisation_in_uk.govspeak.erb) where the wording is identical to what is written here. The only difference is that the partial links to the "Become a British citizen" page on GOV.UK.

There is a case to be made for reusing the partial here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@leenagupte leenagupte self-assigned this Aug 26, 2016
@leenagupte
Copy link
Contributor

leenagupte commented Aug 30, 2016

@robinjam Teeny thing. Can you keep git commit messages to 50/72 characters (see commit 1fdbeaf).

Otherwise 👍 LGTM

@pmanrubia
Copy link
Contributor

pmanrubia commented Aug 30, 2016

@robinjam / @leenagupte another option for keeping the commits atomic could be:

  1. Add the outcome to the flow but copying the same content as in opposite sex.
    cp pathto/outcome_same_sex_marriage_and_civil_partnership_not_possible pathto/outcome_same_sex_civiil_partnership_in_greece
    Update the flow and keep the same content.
    Clarifying in the commit notes that this is a preparatory commit in order to change the copy of the outcome: there should not be any changes to the artefacts (all tests should be green)
  2. Do the copy changes, regenerate artefacts and commit all together. Again a green commit.
  3. Extract partial.

This way we have three atomic (and self-explanatory) commits

@leenagupte
Copy link
Contributor

leenagupte commented Aug 30, 2016

@pmanrubia @robinjam Where a brand new outcome is involved, I don't think it's helpful to initially create the outcome template with the wrong text in it. Personally, I would find that more confusing.

If we wanted to make what's happening clearer, I would add a line of explanation to the commit note. For example, previously it wasn't possible to get a same-sex civil partnership in Greece. This rule was changed... etc

@pmanrubia
Copy link
Contributor

I would prefer having atomic commits where all tests are green in every moment. This approach will help us, for example, to run git bisect at any time, which is always a good thing. In my opinion, the commit history would be clear with my suggestion as we are moving our codebase slowly, in smaller chunks, and always in Green state. And a side note, this would not be the wrong text, it would just be the same status of the app expressed in a different way (the new outcome)

You can find similar (but not identical) examples in:

In any case, this is your call @robinjam and @leenagupte. I don't have strong opinions on the way you have played this card.

@robinjam
Copy link
Contributor Author

@leenagupte @pmanrubia In this particular case the change is so small that it's probably easiest if I just squash this PR down into a single commit.

@robinjam robinjam force-pushed the same-sex-civil-partnerships-in-greece branch from 1fdbeaf to 5522902 Compare August 31, 2016 15:58
@pmanrubia
Copy link
Contributor

I would still prefer having commits split as described in #2708 (comment), but that shouldn't prevent this PR from being merged.

LGTM

@pmanrubia pmanrubia added Passed code review and removed Ready for code review needs content review Waiting for a content designer to approve labels Sep 1, 2016
@robinjam robinjam force-pushed the same-sex-civil-partnerships-in-greece branch 3 times, most recently from 0b93f38 to dbc0687 Compare September 2, 2016 10:48
@robinjam robinjam force-pushed the same-sex-civil-partnerships-in-greece branch from dbc0687 to c31ca15 Compare September 2, 2016 11:39
@robinjam robinjam merged commit 35ec5d4 into master Sep 2, 2016
@robinjam robinjam deleted the same-sex-civil-partnerships-in-greece branch September 2, 2016 12:39
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.

3 participants