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

feat: add feedback feature to self-learning pages #2093

Merged
merged 12 commits into from
Oct 1, 2024

Conversation

gabriele-ct
Copy link
Contributor

@gabriele-ct gabriele-ct commented Sep 25, 2024

Description of changes
Adding feedback functionality connected to userguiding to self-learning pages

Link to original ticket
https://github.com/commercetools/commercetools-docs/issues/4637

Link to change (deep-link for reviewing the change)
https://docskit-ga-userguiding-script-injection.commercetools.vercel.app/self-learning-smoke-test/course-1/2-quiz

Screenshot of changes (if applicable)
Screenshot 2024-09-26 at 14 56 38

DoD guidelines

  • user documentation added?
  • stakeholders approve changes? (if needed)

Copy link

changeset-bot bot commented Sep 25, 2024

🦋 Changeset detected

Latest commit: 0f5daae

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@commercetools-docs/gatsby-theme-docs Minor
@commercetools-website/api-docs-smoke-test Patch
@commercetools-website/docs-smoke-test Patch
@commercetools-website/documentation Patch
@commercetools-website/self-learning-smoke-test Patch
@commercetools-website/site-template Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Sep 25, 2024

@gabriele-ct
Copy link
Contributor Author

@zbalek Would you mind having a look to what implemented so far:
https://docskit-ga-userguiding-script-injection.commercetools.vercel.app/self-learning-smoke-test/course-1/2-quiz

Let me know if you have any observations or you'd like to change anything.
Thank you

@gabriele-ct
Copy link
Contributor Author

I also did some test throttling the network to see how bad is the UX on the first load.
Here are my findings:

  • on a fast connection the modal is pretty much displayed instantaneously
  • on a fast 4G connection it takes about 2 seconds
  • on a slow 4G connection it can take 6 to 8

The good thing is that the user gets an instantaneously feedback (the thumbs icon changes color) and it doesn't feel too bad to wait a couple of seconds for a feeback popup, I've seen some sites delaying the feedback modal on purpose.
Anyway, let me know your thoughts about it

@zbalek
Copy link

zbalek commented Sep 25, 2024

Looks great @gabriele-ct! The behavior is as expected.

Re the loading speed: Thanks for checking this! Imho, this is also really good.

Re mobile: I cross-checked the requirements and we actually haven't defined the situation for mobile (which is when users may be the most likely to access the pages using 4G). And so I was wondering if it is possible to only enable the feedback feature only up to a tablet size screen - at least for the MVP. (I tried to test the mobile version (SE) just now and it is breaking a bit.)


Can we please increase the margin here from 16 to 24:
image

@gabriele-ct gabriele-ct marked this pull request as ready for review September 26, 2024 13:03
@gabriele-ct
Copy link
Contributor Author

@zbalek could you please have another look? I addressed the points you suggested. Thank you

@zbalek
Copy link

zbalek commented Sep 27, 2024

@zbalek could you please have another look? I addressed the points you suggested. Thank you

All good now - thank you!

@gabriele-ct
Copy link
Contributor Author

@FFawzy I haven't been able to reproduce the non loading survey issue, would you mind double check if we're good to merge this?

@FFawzy
Copy link
Contributor

FFawzy commented Sep 27, 2024

@gabriele-ct .. Unfortunately I can still reproduce it.
it happens for fresh new customers please check the video attached
one more thing, I can't find the google tracker on the thumbs events did I miss it ?

userguiding-bug.mov

@gabriele-ct gabriele-ct requested a review from a team September 27, 2024 10:32
@gabriele-ct
Copy link
Contributor Author

Thank you for finding out the repro path. It helped a lot.
Turns out that when the script loads it creates some items in localstorage, one of them looks like the session id, nothing happens until that is present in the localstorage. I'm therefore waiting for that session id to exists before calling any library method. It seems to be working all the times.

I also added google tag calls. It should be ready for a final review 🙇

@FFawzy
Copy link
Contributor

FFawzy commented Sep 30, 2024

Pinging @nkuehn as he is defacto the Self Learning stakeholder for this feature.
can you please test out the feature here
https://docskit-ga-userguiding-script-injection.commercetools.vercel.app/self-learning-smoke-test/course-1/2-quiz

Thank you

@nkuehn
Copy link
Collaborator

nkuehn commented Sep 30, 2024

Oh, thanks! I like it a lot, very nice direct user experience.

I looked at the debug / log a bit, too and it seems we're really only loading userguiding when needed.

Did you check with data protection team whether we they have to do something that the cookie consent banner is maybe re-shown again (or, at least that they have documented that we have userguiding on the docs site now)?

@FFawzy
Copy link
Contributor

FFawzy commented Sep 30, 2024

Oh, thanks! I like it a lot, very nice direct user experience.
I looked at the debug / log a bit, too and it seems we're really only loading userguiding when needed.

Properly specified ✅
properly implemented ✅
🎉

Did you check with data protection team whether we they have to do something that the cookie consent banner is maybe re-shown again (or, at least that they have documented that we have userguiding on the docs site now)?

I'll send them an email to check

Copy link
Contributor

@ColinRosati ColinRosati left a comment

Choose a reason for hiding this comment

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

Looking good. Just a few small comments

@gabriele-ct gabriele-ct requested review from a team and ColinRosati September 30, 2024 14:06
Copy link
Contributor

@ColinRosati ColinRosati left a comment

Choose a reason for hiding this comment

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

LGTM

@gabriele-ct
Copy link
Contributor Author

@FFawzy shall we consider this PR "blocked" until we hear back from data protection team?

@FFawzy
Copy link
Contributor

FFawzy commented Oct 1, 2024

@gabriele-ct we are good to go 🙌

they only needed me to file a report with some details and I did it so we are good to go.

ColinRosati and others added 2 commits October 1, 2024 13:42
… page (#2085)

Description of changes

Align layout header and layout page content for release notes
util for shared layout for header and page content
refactor grid layouts
Shared const IDs in layout components
extracted config and logic
Link to original ticket
closes #2068

* feat: Shared Page Content layout util

* chore: use global grid ids in layouts

* chore: add changeset

* fix: xl column2 bug

* chore: code improvements

* fix: header row height

* fix: column2 tablet

* chore: update comment

* fix: column2 fixed body
@gabriele-ct gabriele-ct merged commit baa10e8 into main Oct 1, 2024
10 checks passed
@gabriele-ct gabriele-ct deleted the ga-userguiding-script-injection branch October 1, 2024 12:14
@ct-changesets ct-changesets bot mentioned this pull request Oct 1, 2024
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