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 callout tour highlights to match design #6973

Closed
mxbclang opened this issue Apr 24, 2023 · 8 comments
Closed

Update callout tour highlights to match design #6973

mxbclang opened this issue Apr 24, 2023 · 8 comments
Labels
Exp: SP Module: Analytics Google Analytics module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature

Comments

@mxbclang
Copy link

mxbclang commented Apr 24, 2023

Description

As reported by Sigal in Bug Bash, the highlights for specific sections of the dashboard do not match the design. For all three:

  1. The highlighted white part is not in the correct size (too large) and placement
  2. The highlighted white part should have a shadow like the tooltip instead of the green highlight it currently has

Note that this is blocked by #6904 and #7001, where we need to update the position of the New badges before modifying the highlights.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The target highlight of the GA4 Reporting feature tour steps should match the Figma design in terms of size and placement. The highlight should have a consistent padding around the target element according to the Figma design, and the target element should be centered inside the highlight.
  • The green outline should be removed from the highlight so that it matches the Figma design.

Implementation Brief

  • In assets/js/feature-tours/ga4-reporting.js:
    • Add a styles key to each item in the steps array. The value of styles should be an object having a spotlight key, with the value being another object with the following keys:
      • border with the value of 0.
      • padding with the value of 15px in the bottom (e.g. 0 0 15px 0).

Test Coverage

  • No tests need to be added/updated.

QA Brief

  • Load the GA4 Feature Tour by having a site with the ga4Reporting feature flag enabled, connecting Analytics, and ensuring the dashboard view used is the GA4 dashboard view.
  • Go to the main dashboard and trigger the feature tour by clicking on the "Learn what's new" CTA button in this notification:

CleanShot 2023-05-15 at 02 19 56

  • The feature tour spotlight should have the correct padding around the elements highlighted; see the Figma links in the issue for reference.

  • This issue also prevents the feature tour from running on mobile viewports. If you follow the QA steps here on mobile, you should see a notification with only an "OK, Got it!" button that dismisses the notification temporarily but does not mark the tour as complete, meaning if you type sessionStorage.clear() in the browser JS console and refresh the page, you will see the notification again and can run the tour once switched to a desktop viewport.

Changelog entry

  • Update GA4 Reporting feature tour highlights to match design.
@mxbclang mxbclang added P1 Medium priority Type: Enhancement Improvement of an existing feature Module: Analytics Google Analytics module related issues P1 High labels Apr 24, 2023
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Apr 27, 2023
@eugene-manuilov eugene-manuilov self-assigned this May 2, 2023
@eugene-manuilov
Copy link
Collaborator

AC ✔️

@wpdarren
Copy link
Collaborator

wpdarren commented May 3, 2023

I am adding an observation that I discovered while testing #6904.

Now that we have made the changes to the badge placement, we've created a UI issue with the tooltip tour. As you can see from the screenshots below, the tooltip slightly overlaps the text. Is it possible to fix this within this ticket, or, do we have another ticket that it can be fixed in? I can create a new ticket, just conscious this would likely be a new P0.

image
image

@techanvil
Copy link
Collaborator

@wpdarren I think we're OK addressing this here - the first point of the AC should cover it:

  • The target highlight of the GA4 Reporting feature tour steps should match the Figma design in terms of size and placement.

Note that the badges are similarly aligned in the Figma design linked to:

image

image

@nfmohit nfmohit self-assigned this May 9, 2023
@nfmohit nfmohit removed their assignment May 10, 2023
@eugene-manuilov eugene-manuilov self-assigned this May 10, 2023
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@nfmohit nfmohit assigned tofumatt and unassigned nfmohit May 17, 2023
@tofumatt tofumatt assigned nfmohit and unassigned tofumatt May 17, 2023
@nfmohit nfmohit removed their assignment May 17, 2023
@techanvil techanvil assigned techanvil and unassigned techanvil May 19, 2023
@mohitwp mohitwp self-assigned this May 25, 2023
@mohitwp
Copy link
Collaborator

mohitwp commented May 26, 2023

QA Update ❌

@tofumatt

  • Tested on dev environment.
  • Tested tour highlights in desktop and tablet view ports.
  • Verified in mobile view ports "Learn what's new' button is not showing and also tour is not getting trigger.
  • Verified The highlight should have a consistent padding around the target element according to the Figma design, and the target element is now centered inside the highlight.
  • Verified the green outline is now removed from the highlight.

Issue :- I've noticed that in tablet view ports having width between 783px to 1179px, the session target highlight is overlapping the engagement title.

image

Recording.379.mp4

Question – In IB it is mentioned that top padding is 0px and bottom padding is 15px. But under actual implementation top padding is 12px and bottom padding is 20px which looks more correct. Did we implement different padding from the suggested padding in IB so that target element gets centered around the highlight ?

image

image

image

@mohitwp mohitwp assigned tofumatt and unassigned mohitwp May 26, 2023
@tofumatt
Copy link
Collaborator

@mohitwp Yes, the padding is different to look better; it's always best to compare the ACs not the IB, as sometimes we deviate a bit 😄

Thanks for catching the tablet padding, I'll get a follow-up PR for that one 👍🏻

@tofumatt
Copy link
Collaborator

@mohitwp I just spent a fair bit of time on this, trying to get the widths to align perfectly on smaller screens, but because of React Joyride's own logic around spotlights and those headers being VERY small, it doesn't seem to work.

For now I think the spotlight being wider than the contents on smaller screens will need to be a known bug, so moving this to Approval as the QA was otherwise good and I think that width on tablets is a "Won't fix" issue, at least in the context of this issue.

@tofumatt tofumatt removed their assignment May 30, 2023
@aaemnnosttv
Copy link
Collaborator

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP Module: Analytics Google Analytics module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants