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

Merge contacts frontend into government frontend #253

Merged
merged 27 commits into from
Mar 2, 2017
Merged

Conversation

mcgoooo
Copy link
Contributor

@mcgoooo mcgoooo commented Feb 21, 2017

This is the first part of the template consolidation work. This renders contacts frontend in government frontend.

part of :- https://trello.com/c/0Ree9gt0/140-3-render-contacts-in-government-frontend-l

it handles all the rendering, and Webchat that was handled by contacts frontend. Please note, the webchat was taken in as is, as it is being handled in firebreak

The breadcrumbs are awaiting on work from the finding things team.

the contacts template is using govspeak css and will be having a going over with the content team to make it more consistent. the reason Govspeak was used was to introduce more consistency as opposed to specific styles per template.

you can see changes supplied by wraith here https://template-consolidation-contacts-diff.surge.sh/

this pull is dependent on updating the schemas

Before After
multi_phantomjs_local multi_phantomjs_production

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-253 February 21, 2017 16:51 Inactive
@boffbowsh
Copy link
Contributor

This didn't previously have the beta banner and I can't easily see a description of why and where it got added.

@NickColley
Copy link
Contributor

NickColley commented Feb 22, 2017

We removed the beta banner since no product team is iterating contacts at the moment. So it felt like something that should have been removed a long time ago.

I'd be happy to put it back if there's a good reason to though. 👍

Edit: Just noticed the screenshots were in the wrong order. :)

//= require_tree ./modules
//= require webchat.js
Copy link
Contributor

@fofr fofr Feb 22, 2017

Choose a reason for hiding this comment

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

Could we include this only on pages that have webchat? This would other wise be downloaded for every format.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be 👍 for that, seems like a good thing.

@fofr
Copy link
Contributor

fofr commented Feb 22, 2017

Can the commit message for 3cc1e1c be updated with a link to the history of the original webchat feature?

end


test "post are rendered" do
Copy link
Contributor

Choose a reason for hiding this comment

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

"post are rendered" (also, whitespace)

end

test 'presents the format' do
assert_equal schema_item['format'], presented_item.format
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this test

end

test 'phone returns correctly' do
assert_equal schema_item['details']['phone_numbers'][0]["number"], presented_item.phone[0][:numbers][0][:number]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd save schema_item['details']['phone_numbers'][0] in a variable

assert_not_nil html.at_css(".street-address")
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no test that the related links component is rendered with the correct parameters.

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-253 February 23, 2017 11:10 Inactive
@fofr
Copy link
Contributor

fofr commented Feb 27, 2017

What's holding this up?

It also needs a rebase with master after travel advice was added.

@NickColley
Copy link
Contributor

Just me trying to find time to update this PR, I'm close to getting in a few commits that address the rest of your comments. ⏲

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-253 February 27, 2017 15:28 Inactive
@boffbowsh boffbowsh had a problem deploying to government-frontend-pr-253 February 27, 2017 15:57 Failure
@boffbowsh boffbowsh had a problem deploying to government-frontend-pr-253 February 27, 2017 15:58 Failure
@NickColley
Copy link
Contributor

I've updated the PR so that Webchat is only loaded when it's needed. We should test this in production mode that the webchat payload is only loaded when needed etc.

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-253 February 27, 2017 16:23 Inactive
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-253 February 27, 2017 17:16 Inactive
@NickColley NickColley changed the title [do not merge] Merge contacts frontend into government frontend Merge contacts frontend into government frontend Feb 28, 2017
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-253 March 1, 2017 12:08 Inactive
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-253 March 1, 2017 12:12 Inactive
README.md Outdated
@@ -17,6 +17,7 @@ Not all formats that this app can handle are rendered by it in production.
| Case study | [View on GOV.UK](https://www.gov.uk/government/case-studies/2013-elections-in-swaziland) | Migrated |
| Coming soon | | Rendered by Whitehall |
| Consultation | [View on GOV.UK](https://www.gov.uk/government/consultations/soft-drinks-industry-levy) | Migrated |
| contacts | [View on GOV.UK](https://www.gov.uk/government/organisations/hm-revenue-customs/contact/alcohol-duties-national-registration-unit) | Currently rendered by Contacts frontend |
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Should be uppercase

@fofr
Copy link
Contributor

fofr commented Mar 1, 2017

For 7f2cf72 can you also revert the changes to application.js: https://github.com/alphagov/government-frontend/pull/253/files#diff-a9c3bd311eab80c9ebe6a69830f9ad02

(Also, there's a typo in commit)

webchat is a legacy module that is not written in the
standard GOV.UK module pattern.

This module is going to be looked at being rewritten in firebreak.
while just calling it legacy and been done with it is not ideal,
we are looking at it in short order.

Also add contacts to readme and revert the change to application.js
back to just include the modules directory
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-253 March 1, 2017 16:55 Inactive
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.

5 participants