Adding NPS rating for docs pages

[asaadmahmood]

Summary

Adding NPS rating for docs pages

Ticket Link

https://mattermost.atlassian.net/browse/MM-23098

[justinegeffen]

Thanks @asaadmahmood, this is great! :)

Next step is that I am awaiting feedback from the Analytics team to confirm that I can add the events to our GA account. Adding a screenshot from my local build for reference as I am still setting up my web server.

[jasonblais]

@asaadmahmood @justinegeffen Before merging, propose considering the following:

1. Does a floating emoji icon on the page make sense or if it should be a more generic "feedback" icon (the ratings themselves can be emojis)
2. If 4 rating options is the right number - from research, it seems it's often 2-3 options: https://community.mattermost.com/private-core/pl/pn971u37ntdaprcq1gnshjfk5c

Else, excited to see this progress 🎉

Also, this is for the rating score only, no text-based answers for now?

[asaadmahmood]

@jasonblais

1. Currently we don't have one icon, we just have those 4 icons floating around. So they occupy more space, yet are very apparent.
2. We can go with 3.

Yes, for the score only, we can expand on this later.

[justinegeffen]

Good questions, @jasonblais. I suggested to @asaadmahmood that we start with just emoji ratings to ensure the events are properly tracked by Google Analytics. Once we have confirmed this, the next iteration can include an on-page comment box (workflow for this has not yet been defined).

I think four provides a bigger range of feedback but by the same token it might be easier to start with three ratings and then refine them. 3/5.

[justinegeffen]

I've added the event tracking code to the docs based on the content here: https://developers.google.com/analytics/devguides/collection/gtagjs/events. Next step is a review of the code and steps to validate. I've asked Kendall to take a look at it as I am getting an error when I build locally (not sure if that's format-related):

updating environment: [config changed] 338 added, 6 changed, 0 removed
/Users/justinegeffen/.local/share/virtualenvs/docs-qocg8X3N/lib/python3.7/site-packages/recommonmark/parser.py:65: UserWarning: Container node skipped: type=document                                                
  warn("Container node skipped: type={0}".format(mdnode.t))

[justinegeffen]

@ztrayner, I chatted to Kendall and she recommended that I get your review and next steps on this if you have the bandwidth. :)

[justinegeffen]

Following up on this, @ztrayner, to find out if there's anything I can do on my side wrt testing. :)

[ztrayner]

@justinegeffen looks good, just be aware that you can sometimes run into data sampling issues with GA. This should give us a good idea though.

[justinegeffen]

@justinegeffen looks good, just be aware that you can sometimes run into data sampling issues with GA. This should give us a good idea though.

Brilliant, thank you! :)

[justinegeffen]

This looks good to me, thank you!

[justinegeffen]

@asaadmahmood, I'm getting a build error when I build the docs, but it doesn't look like there are any errors on the page. Would you be open to checking the GA code before we merge this?

[justinegeffen]

@amyblais, I was wondering if you'd be able to take a look at this and let me know where I went wrong with the new code and why it's causing the build error? I think it's otherwise good to merge but don't want to break anything. :)

[asaadmahmood]

@justinegeffen Can you show me the build error here?

[justinegeffen]

hey @asaadmahmood, sure, it's this:

updating environment: [config changed] 338 added, 6 changed, 0 removed
/Users/justinegeffen/.local/share/virtualenvs/docs-qocg8X3N/lib/python3.7/site-packages/recommonmark/parser.py:65: UserWarning: Container node skipped: type=document
warn("Container node skipped: type={0}".format(mdnode.t))

And here it is in my terminal window:

[asaadmahmood]

@justinegeffen I see that error on master aswell, are you sure you only see that error on this branch?

[justinegeffen]

@justinegeffen I see that error on master aswell, are you sure you only see that error on this branch?

Great question, I didn't actually check that. Let me do another build.

[justinegeffen]

Yah I see it on master too so I guess it's not related to these changes. Thanks for picking that up!

I think we should be good to merge once @amyblais has had a look. :) Thanks @asaadmahmood!

[amyblais]

I don't have enough knowledge to review / test this.

[justinegeffen]

I don't have enough knowledge to review / test this.

Thanks @amyblais - I chatted to @jasonblais and I think we're good to go. :) Appreciate the feedback. :)