Display GOV.UK Chat promo on certain pages #3303

jackbot · 2024-08-19T11:10:34Z

Note: this is a draft and an incomplete implementation. I'm just looking for feedback for the approach taken.

We want to show a promo to the new GOV.UK Chat application on certain pages on GOV.UK. It's a CTA pointing users to the chat app.

The list of URLs we want the promo to appear on is quite short and cover only answer and guide content types from this codebase.

The CTA is to be shown in the sidebar on the right of the relevant pages.

kevindew

Approach looks great to me, looks like @jon-kirwan isn't far away from merging the component in so that we can see the final result: alphagov/govuk_publishing_components#4151

Not sure if you and him want to converge to try out his component before he merges it to rule out any issues?

config/application.rb

app/views/shared/_sidebar_navigation.html.erb

kevindew · 2024-08-20T08:10:17Z

Nice one, this looks good enough to me that we could proceed to write tests for it.

test/helpers/govuk_chat_promo_helper_spec.rb

test/support/govuk_chat_promo.rb

jackbot · 2024-08-20T14:09:56Z

@kevindew I've added some tests here, with a couple of comments.

I tested out the branch of govuk_publishing_components with the actual component in and noticed a couple of discrepancies between what was rendered and the Figma designs. I've chatted with Jon about it, and he'll look into it.

The tests are written in such a way that they'll still be relevant when I switch out the dummy component with the real one, as they both contain the text "Try GOV.UK Chat"

kevindew

Thanks @jackbot - nice job with the minitest fight. After chatting with @leenagupte I've made a few suggestions that are related to how we could simplify things a bit. They were a little surprised at how many files are affected, which is my fault as I told them it'd not be much more complex than:

<% if SHOW_CHAT_PROMO_BASE_PATHS.include?(content_item.base_path) %>
  <%= render "govuk_publishing_components/chat_promo" %>
<% end %>

Let me know what you think, but I think in general they'll make it mostly easier to test and reduce places with chat code.

app/helpers/govuk_chat_promo_helper.rb

app/views/shared/_sidebar_navigation.html.erb

test/helpers/govuk_chat_promo_helper_spec.rb

test/support/govuk_chat_promo.rb

jackbot · 2024-08-21T07:38:01Z

Thanks @kevindew, I've made those changes to simplify things a bit.

jackbot · 2024-08-21T07:44:30Z

test/integration/answer_test.rb

@@ -33,4 +33,12 @@ class AnswerTest < ActionDispatch::IntegrationTest
    setup_and_visit_content_item("answer")
    assert_not page.has_css?(".gem-c-single-page-notification-button")
  end
+
+  test "renders GOV.UK chat promo for matching base path" do


Wasn't really sure where to put this test now that we could technically render the promo on any content type page now. There doesn't seem to be any "integration test for all content types" test file anywhere, as far as I could see.

Yeah it doesn't look like there is one. Closest thing seems to be meta_tags test.

I imagine we've got a choice between

no test, consider the integration code sufficiently simple that it doesn't need a test

create a simple govuk_chat_promo_test in integration tests.

I think the most responsible thing to do is the latter with us being prepared to remove it if the team don't want it to keep chat surface area smaller.

I guess a thing to consider is that, for when we have the component installed, the assertion will change from has_text?("Try GOV.UK Chat") to has_selector(".gem-c-chat-entry") so would be a little more contrived as we shouldn't couple the test to the component copy.

Hello. I agree that 2, creating a separate test file is the best option. Keeping the tests separate will make them easier to remove later without needing to unpick them from the rest of the test suite.

Thanks both, I've created a separate test file that just tests the happy path; I think the unit tests for the helper cover all the rest of the paths but let me know if you disagree.

I've also removed the temporary view because we can now render the actual component.

kevindew

Great stuff, just a note on the integration test

kevindew · 2024-08-21T11:33:38Z

test/helpers/govuk_chat_promo_helper_spec.rb

+
+class GovukChatPromoHelperTest < ActionView::TestCase
+  test "show_govuk_chat_promo? when configuration disabled" do
+    assert_not show_govuk_chat_promo?(GOVUK_CHAT_PROMO_BASE_PATHS.first)


Aside: It surprises me that this constant is in scope, I wonder how that works. I didn't think it'd be included when methods are included.

This was a surprise to me too.

kevindew · 2024-08-21T11:40:36Z

test/integration/answer_test.rb

@@ -33,4 +33,12 @@ class AnswerTest < ActionDispatch::IntegrationTest
    setup_and_visit_content_item("answer")
    assert_not page.has_css?(".gem-c-single-page-notification-button")
  end
+
+  test "renders GOV.UK chat promo for matching base path" do


Yeah it doesn't look like there is one. Closest thing seems to be meta_tags test.

I imagine we've got a choice between

no test, consider the integration code sufficiently simple that it doesn't need a test

create a simple govuk_chat_promo_test in integration tests.

I think the most responsible thing to do is the latter with us being prepared to remove it if the team don't want it to keep chat surface area smaller.

I guess a thing to consider is that, for when we have the component installed, the assertion will change from has_text?("Try GOV.UK Chat") to has_selector(".gem-c-chat-entry") so would be a little more contrived as we shouldn't couple the test to the component copy.

jackbot · 2024-08-27T08:57:00Z

@kevindew @leenagupte one thing I've noticed is the placement of the CTA on mobile in the designs makes things a little bit more complex.

Here's a side-by-side of what it currently looks like vs what it should look like:

App	Design

So the CTA should come below the related content, whereas the placement in this PR places it above, like it does on desktop.

So there'a couple of solutions here:

Leave it as it is - means less impact on this codebase. We can talk to design and see if they're happy with this compromise
Modify the CSS so that on mobile the sidebar is display: flex; flex-direction: column-reverse to swap the position of the CTA and related content. This comes at the cost of touching more files in this codebase though

Let me know what you think.

leenagupte · 2024-08-27T09:28:49Z

@jackbot I think it's best to check with the frontend community about how the stacking of components works and the best options for solving this problem. Perhaps @andysellick or @AshGDS could help?

AshGDS · 2024-08-27T10:07:09Z

If I'm understanding the flex proposal correctly, would this swap the order of the Related content and Collection sections on the sidebar? If so, this will impact analytics (which relies on indexes listing what position the sections are in lists)

kevindew · 2024-08-27T10:23:22Z

Thanks @jackbot, so if I understand correctly the designs dictate a different ordering for desktop and mobile. I think it'd be best to check with our designers first on this. Let's first rule out that the changing order isn't a mistake. It seems unnecessarily complex having order different in mobile so I think we should push for having just one that is consistent for both.

jackbot · 2024-08-27T10:26:02Z

@AshGDS thanks for the reply. Sorry, my screenshots didn't illustrate this particularly well.

The flex proposal wouldn't touch the ordering of the "Related content" and "collection" sections at all. It would position the new Chat promo below the "collection" section.

As-is	With `flex` stuff applied

jackbot · 2024-08-27T10:27:22Z

Thanks @kevindew, I'll check with design then.

jon-kirwan · 2024-08-27T10:32:26Z

Modify the CSS so that on mobile the sidebar is display: flex; flex-direction: column-reverse to swap the position of the CTA and related content. This comes at the cost of touching more files in this codebase though

Let me know what you think.

I think there could be accessibility problems with changing the order using flex, particularly for keyboard users, as it will cause items to be navigated in reverse order.

AshGDS · 2024-08-27T10:33:19Z

No worries, thanks @jackbot - an issue with ordering elements with flex is that the order of the DOM is preserved, which could create an accessibility issue.

See here for more information: https://adropincalm.com/blog/accessibility-issues-using-order-in-grid-and-flex/

EDIT: Ah, just seen Jon has said the same 👍

jackbot · 2024-08-27T13:08:44Z

Thanks for that @jon-kirwan and @AshGDS, I wasn't aware of that accessibility issue.

I've talked to design about this and they're happy that the promo sits above the related content section, so we don't need to do anything here to fiddle with the layout on mobile.

I'm going to mark this ready for review now.

leenagupte

I've added one tiny inline comment about the test file name, but other than that it looks good to me 👍

I've added the environment variable to the review app in Heroku to try it out.

Strangely enough the first link https://government-frontend-pr-3303.herokuapp.com/business-support-helpline redirects to https://www.gov.uk/business-support-service for me. Not sure what's happening there.

All the other pages look great though.

test/helpers/govuk_chat_promo_helper_spec.rb

leenagupte · 2024-08-27T14:59:58Z

test/helpers/govuk_chat_promo_helper_spec.rb

+
+class GovukChatPromoHelperTest < ActionView::TestCase
+  test "show_govuk_chat_promo? when configuration disabled" do
+    assert_not show_govuk_chat_promo?(GOVUK_CHAT_PROMO_BASE_PATHS.first)


This was a surprise to me too.

We want to show a promo to the new GOV.UK Chat application on certain pages on GOV.UK. The list of URLs is quite short and cover only `answer` and `guide` content types. The CTA is to be shown in the sidebar on the right of the relevant pages.

jackbot · 2024-08-28T07:21:06Z

Thanks for adding the env var there and doing the testing @leenagupte. I've fixed the filename of the test file now.

leenagupte

👍

We want to show a promo to the new GOV.UK Chat application on certain pages on GOV.UK. We already have this implemented in Government Frontend alphagov/government-frontend#3303 and Collections alphagov/collections#3742. We're implementing it in: - finder_frontend - smart_answers - frontend This updates the finder show page to render the chat banner above the related links similar to the other applications.

govuk-ci temporarily deployed to government-frontend-pr-3303 August 19, 2024 11:10 Inactive

kevindew reviewed Aug 19, 2024

View reviewed changes

config/application.rb Outdated Show resolved Hide resolved

app/views/shared/_sidebar_navigation.html.erb Outdated Show resolved Hide resolved

govuk-ci temporarily deployed to government-frontend-pr-3303 August 20, 2024 07:22 Inactive

govuk-ci temporarily deployed to government-frontend-pr-3303 August 20, 2024 14:01 Inactive

jackbot commented Aug 20, 2024

View reviewed changes

test/helpers/govuk_chat_promo_helper_spec.rb Outdated Show resolved Hide resolved

jackbot commented Aug 20, 2024

View reviewed changes

test/support/govuk_chat_promo.rb Outdated Show resolved Hide resolved

kevindew reviewed Aug 20, 2024

View reviewed changes

jackbot force-pushed the govuk-chat-promo branch from 73cea68 to 250f45c Compare August 21, 2024 07:32

govuk-ci temporarily deployed to government-frontend-pr-3303 August 21, 2024 07:32 Inactive

jackbot force-pushed the govuk-chat-promo branch from 250f45c to 80c18c8 Compare August 21, 2024 07:40

govuk-ci temporarily deployed to government-frontend-pr-3303 August 21, 2024 07:40 Inactive

jackbot commented Aug 21, 2024

View reviewed changes

kevindew reviewed Aug 21, 2024

View reviewed changes

jackbot force-pushed the govuk-chat-promo branch from 80c18c8 to 56b8d8e Compare August 27, 2024 08:48

govuk-ci temporarily deployed to government-frontend-pr-3303 August 27, 2024 08:49 Inactive

jackbot marked this pull request as ready for review August 27, 2024 13:08

leenagupte reviewed Aug 27, 2024

View reviewed changes

Display GOV.UK Chat promo on certain pages

abd4912

We want to show a promo to the new GOV.UK Chat application on certain pages on GOV.UK. The list of URLs is quite short and cover only `answer` and `guide` content types. The CTA is to be shown in the sidebar on the right of the relevant pages.

jackbot force-pushed the govuk-chat-promo branch from 56b8d8e to abd4912 Compare August 28, 2024 07:18

govuk-ci temporarily deployed to government-frontend-pr-3303 August 28, 2024 07:18 Inactive

jackbot changed the title ~~[DO NOT MERGE] Display GOV.UK Chat promo on certain pages~~ Display GOV.UK Chat promo on certain pages Aug 28, 2024

leenagupte approved these changes Aug 28, 2024

View reviewed changes

jackbot merged commit d8751d8 into main Aug 28, 2024
11 checks passed

jackbot deleted the govuk-chat-promo branch August 28, 2024 09:45

davidgisbey mentioned this pull request Nov 25, 2024

Render GOV.UK chat promotional banner on specific pages alphagov/frontend#4453

Merged

jackbot mentioned this pull request Nov 26, 2024

Render GOV.UK Chat promo on specific page alphagov/smart-answers#6961

Merged

davidgisbey mentioned this pull request Nov 26, 2024

Add GOV.UK Chat promo banner to business finance support finder alphagov/finder-frontend#3574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display GOV.UK Chat promo on certain pages #3303

Display GOV.UK Chat promo on certain pages #3303

jackbot commented Aug 19, 2024 •

edited

Loading

kevindew left a comment

kevindew commented Aug 20, 2024

jackbot commented Aug 20, 2024

kevindew left a comment

jackbot commented Aug 21, 2024

jackbot Aug 21, 2024

kevindew Aug 21, 2024

leenagupte Aug 22, 2024

jackbot Aug 27, 2024

kevindew left a comment

kevindew Aug 21, 2024

leenagupte Aug 27, 2024

kevindew Aug 21, 2024

jackbot commented Aug 27, 2024

leenagupte commented Aug 27, 2024

AshGDS commented Aug 27, 2024

kevindew commented Aug 27, 2024

jackbot commented Aug 27, 2024

jackbot commented Aug 27, 2024

jon-kirwan commented Aug 27, 2024

AshGDS commented Aug 27, 2024 •

edited

Loading

jackbot commented Aug 27, 2024

leenagupte left a comment

leenagupte Aug 27, 2024

jackbot commented Aug 28, 2024

leenagupte left a comment

Display GOV.UK Chat promo on certain pages #3303

Display GOV.UK Chat promo on certain pages #3303

Conversation

jackbot commented Aug 19, 2024 • edited Loading

kevindew left a comment

Choose a reason for hiding this comment

kevindew commented Aug 20, 2024

jackbot commented Aug 20, 2024

kevindew left a comment

Choose a reason for hiding this comment

jackbot commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackbot commented Aug 27, 2024

leenagupte commented Aug 27, 2024

AshGDS commented Aug 27, 2024

kevindew commented Aug 27, 2024

jackbot commented Aug 27, 2024

jackbot commented Aug 27, 2024

jon-kirwan commented Aug 27, 2024

AshGDS commented Aug 27, 2024 • edited Loading

jackbot commented Aug 27, 2024

leenagupte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackbot commented Aug 28, 2024

leenagupte left a comment

Choose a reason for hiding this comment

jackbot commented Aug 19, 2024 •

edited

Loading

AshGDS commented Aug 27, 2024 •

edited

Loading