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

Update KB helpful survey #6423

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Conversation

akatsoulas
Copy link
Collaborator

No description provided.

Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

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

The use of htmx and alpine.js is pretty cool!

  • The GA4 tracking events are not being generated (see comments in code below).

  • When the user is not authenticated but has already voted anonymously, the voting form is still displayed when the article is viewed again, and if another vote is attempted, the response from the vote POST is a redirect to the KB article, which causes a strange article-within-an-article view of the page.

image
  • The font size and weight of the Yes and No of the voting form should match those of its Was this article helpful? text.
image
  • When the user is authenticated, the voting form is pushed way down on the page.
image
  • The Cancel and Submit buttons of both the "yes" and "no" surveys are stacked instead of side-by-side.
image
  • When submitting the survey, the response is Thanks for making us better! instead of Thanks for your feedback! as the Figma indicates. Also, the text is not bold as is shown in Figma.

kitsune/sumo/static/sumo/js/wiki.metrics.js Outdated Show resolved Hide resolved
kitsune/sumo/static/sumo/js/questions.js Outdated Show resolved Hide resolved
kitsune/sumo/static/sumo/js/wiki.metrics.js Outdated Show resolved Hide resolved
@akatsoulas
Copy link
Collaborator Author

akatsoulas commented Jan 8, 2025

* The font size and weight of the `Yes` and `No` of the voting form should match those of its `Was this article helpful?` text.
image

I am not sure about this. We treat the Was this article helpful as a header. I will follow up with Josh on this.

* When the user is authenticated, the voting form is pushed way down on the page.

It's actually the same space with the logged out view. The difference is that the editing tools push down the whole section

* The `Cancel` and `Submit` buttons of both the "yes" and "no" surveys are stacked instead of side-by-side.

Have you tried a fresh build? It seems to be working for me
image

@escattone
Copy link
Contributor

It's actually the same space with the logged out view. The difference is that the editing tools push down the whole section

Ah, I see that now, thanks. Ignore that comment! 😄

Regarding the cancel and submit buttons of the survey, I did have a fresh build, but they're still always stacked for me. However, I noticed that your survey is wider than mine, so I guess the width of my survey is what is causing the buttons to stack. Hm, I'm not sure why my survey has a smaller width. Anyway, ignore that one too.

image

@akatsoulas
Copy link
Collaborator Author

It's actually the same space with the logged out view. The difference is that the editing tools push down the whole section

Ah, I see that now, thanks. Ignore that comment! 😄

Regarding the cancel and submit buttons of the survey, I did have a fresh build, but they're still always stacked for me. However, I noticed that your survey is wider than mine, so I guess the width of my survey is what is causing the buttons to stack. Hm, I'm not sure why my survey has a smaller width. Anyway, ignore that one too.

I will have another look to make sure that it scales properly to different screen sizes

Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

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

Overall, I really like the GA4-event-related changes, and when I tested them locally, the GA4 events are generated properly (except for the first comment below). I have two comments:

  • After submitting a survey response, a final pair of survey_opened and survey_closed GA4 events are generated, because survey_form.html is rendered and returned for the response_message of Thanks for your feedback!. Both of those survey events include an empty survey_type parameter. See the image below. I'm thinking we shouldn't generate survey events in this case, because it'll misleadingly inflate the GA4 survey events.
image
  • For the survey events, maybe we should use article_survey_... instead of survey_... to distinguish these events from any possible future survey events?

Comment on lines +365 to +372
<button type="submit" class="btn helpful-button" name="helpful"
data-event-name="article_vote"
data-event-parameters='{"vote": "helpful"}'
>{{ _('Yes') }}👍</button>
<button type="submit" class="btn not-helpful-button" name="not-helpful"
data-event-name="article_vote"
data-event-parameters='{"vote": "not-helpful"}'
>{{ _('No') }}👎</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

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

The GA4 events look good now. Just an indentation nit, and there's still the issue of the text of the Yes👍 and No👎 buttons looking different than the Figma designs (but as you said, you can discuss that with Josh). Thanks @akatsoulas!

Comment on lines 29 to 32
const surveyType = '{{ survey_type }}'.trim();
if (surveyType) {
trackEvent('article_survey_opened', { survey_type: surveyType });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. The indentation is off.

@akatsoulas akatsoulas merged commit 0ce0ad7 into mozilla:main Jan 9, 2025
2 checks passed
@akatsoulas akatsoulas deleted the update-kb-survey branch January 9, 2025 17:24
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