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

Add the Audience Tile happy path view (Storybook) #8135

Closed
9 tasks done
techanvil opened this issue Jan 24, 2024 · 14 comments
Closed
9 tasks done

Add the Audience Tile happy path view (Storybook) #8135

techanvil opened this issue Jan 24, 2024 · 14 comments
Labels
Good First Issue Good first issue for new engineers Module: Analytics Google Analytics module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jan 24, 2024

Feature Description

Create the individual audience tile component, implementing the happy path view (i.e. the main view showing the metrics for an audience), and add it to Storybook.

Note this does not include the loading state or any other state variants which will be implemented separately.

See audience tiles > individual tiles in the design doc.


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

Acceptance criteria

  • A new component along with its Storybook story should be added for audience tile (happy path).
  • Appropriate props should be added as per the design doc.
  • The UI should be similar to the Figma designs.
  • The component should follow the relevant Figma designs at mobile breakpoints (see the phone and tablet designs).
    • On phones and tablets, the title is part of a tabbed component.

Implementation Brief

  • Add assets/js/modules/analytics-4/components/widgets/AudienceSegmentation/AudienceTile.js
    • Include the following props:
      • title
      • infoTooltip
      • visitors
      • visitsPerVisitors
      • pagesPerVisit
      • pageviews
      • topCities
      • topContent
      • Widget
  • visitors, visitsPerVisitors, pagesPerVisit, and pageviews props, should receive value as an object. They will be extracted from report response in parent component, and will represent the metric values. The object should look like this {previousValue: x, currentValue: x, metricValue: x}. Mimic their usage to what we have in MetricTileNumeric component.
  • topCities and topContent will receive an object in format {dimensionValues: [], metricValues: [], total: x }. It will represent the data that is used in TopCitiesWidget component for example, and then you can borrow the logic from MetricTileTable component
  • Wrap the component output with <Widget noPadding>, and a main child div with class, say googlesitekit-audience-segmentation-tile
  • Borrow the logic for infoTooltip components from one of the key metrics widgets
  • Tile title and info tooltip should be grouped under same div wrapper
  • Include additional badge in Top content section - currently rendered unconditionally as it is not in scope of this issue, and match the styling and content Partial data with figma design.
    • Also handle the case if topContent or topCities prop are null, a message No data to show yet should be displayed instead of their data
  • Use useBreakpoint hook to extract the current device size, so that tile title with infoTooltip are conditionally rendered in following manner:
    • in case of BREAKPOINT_TABLET or BREAKPOINT_SMALL title should not be rendered
    • Otherwise show title
  • Add assets/sass/components/analytics-4/audience-segmentation/audience-tile.scss and apply the styles to match the figma designs linked in AC for desktop and mobile

Test Coverage

  • Add AudienceTile tests and stories

QA Brief

  • View the new component in storybook confirming it matches the design doc and mockups at all breakpoints.

Changelog entry

  • Add the Audience Tile in its "happy path" state as a component which is viewable in Storybook.
@techanvil techanvil added Module: Analytics Google Analytics module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature labels Jan 24, 2024
@asvinb asvinb assigned asvinb and unassigned asvinb Jan 31, 2024
@eugene-manuilov eugene-manuilov self-assigned this Jan 31, 2024
@eugene-manuilov
Copy link
Collaborator

@techanvil, why do we have a separate task for this storybook story? It should be part of the #8131 ticket, shouldn't it? We usually create new stories when we work on actual components. Or I miss something?

@techanvil
Copy link
Collaborator Author

techanvil commented Jan 31, 2024

@eugene-manuilov, #8131 only relates to the UI and logic encapsulated in the Setup CTA Banner component, whereas this issue is about introducing the individual Audience Tile component, initially in Storybook.

@eugene-manuilov
Copy link
Collaborator

@eugene-manuilov, #8131 only relates to the UI and logic encapsulated in the Setup CTA Banner component, whereas this issue is about introducing the individual Audience Tile component, initially in Storybook.

Thanks, @techanvil, I was confused by what it means (Storybook) in the title. So, basically we need to implement the corresponding component, but not connect it to the plugin UI yet.

  • A new Storybook story should be added for the individual audience tile component as per the design doc.

@asvinb, let's probably rephrase AC a little bit because currently it is confusing as it feels like the main purpose of this task is to create a storybook story. AC should say that first of all we need to create the new component Audience Tile and corresponding storybook story(ies) for it.

@techanvil
Copy link
Collaborator Author

Thanks, @techanvil, I was confused by what it means (Storybook) in the title. So, basically we need to implement the corresponding component, but not connect it to the plugin UI yet.

That's right @eugene-manuilov. Thanks for pointing out the confusion, I can see what you mean. I've updated the Feature Descriptions for all of the issues that involve creating a component and adding it to Storybook to make them clearer.

@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label Feb 5, 2024
@asvinb
Copy link
Collaborator

asvinb commented Feb 8, 2024

Thanks @eugene-manuilov . Let me know if it's clearer now. Thanks!

@asvinb asvinb assigned eugene-manuilov and unassigned asvinb Feb 8, 2024
@eugene-manuilov
Copy link
Collaborator

Yes, thanks, Asvin. AC ✔️

@eugene-manuilov eugene-manuilov removed their assignment Feb 8, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Feb 8, 2024
@eugene-manuilov eugene-manuilov self-assigned this Feb 9, 2024
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Feb 9, 2024
@bethanylang bethanylang removed the Next Up Issues to prioritize for definition label Feb 12, 2024
@benbowler benbowler self-assigned this Mar 11, 2024
@benbowler benbowler removed their assignment Mar 15, 2024
@wpdarren wpdarren added the Good First Issue Good first issue for new engineers label Mar 18, 2024
@techanvil techanvil self-assigned this Mar 18, 2024
@benbowler
Copy link
Collaborator

Hey @kelvinballoo, thanks for your QA.

RE Issue 1, 2 and 6, the total width or your storybook window must be >1200px to load the desktop breakpoint, in these issues you were viewing the mobile or tablet breakpoint where these elements should be missing or padding/layout is different.

Screenshot 2024-04-02 at 16 38 54

RE Issue 3, I've manually set the pixel line-heights of the text to match the height for each metric section.

RE Issue 4 and 5, fixed.

RE Issue 7, is consistent with other usage of digits of there range in the Key Metrics widgets and other dashboard widgets.

RE Issue 8 and 9, fixed.

@kelvinballoo
Copy link
Collaborator

QA Update ❌

Thanks @benbowler , great work.

Retested as follows and I just need you to look into Issues 2, 9, 10.

ISSUE 1 :
Looking good with a 24px margin after enlarging the viewport.

Screenshot

24px margin on the sides

Screenshot 2024-04-03 at 16 56 30
__________________________________________________________________

ISSUE 2 :
We'll still need to look into issue 2 as the tooltip is not present.
Also, the 'V' in 'Visitors' should be lower case.

Screenshots

Implementation

Screenshot 2024-04-03 at 16 59 29

Figma

Screenshot 2024-04-03 at 16 59 37
__________________________________________________________________

ISSUE 3 ✅
The sections are now 76px.

Screenshot

52+12+12 = 76 px

Screenshot 2024-04-03 at 17 01 52
__________________________________________________________________

ISSUE 4 ✅
The margin is now 20px, not 19.

Screenshot

20px on the right of the icon

Screenshot 2024-04-03 at 17 11 32
__________________________________________________________________

ISSUE 5 ✅
The icon is not vertically centred.

Screenshot Screenshot 2024-04-03 at 17 11 36
__________________________________________________________________

ISSUE 6 ✅
Retesting on desktop viewport, the margin now appears accordingly and is 16px.

Screenshot Screenshot 2024-04-03 at 17 16 11
__________________________________________________________________

ISSUE 7
Noted on the approach of making this consistent with other sections.


ISSUE 8
All the mini-issues have been resolved.


ISSUE 9
All the mini-issues have been resolved except for the bottom gap.
While we do have a 12 px gap, we have an extra 16px gap on top.
Refer to the screenshots below:

Screenshots

Figma: 12+16

Screenshot 2024-04-03 at 17 36 45

Implementation: only 12 (+4 from the text)

Screenshot 2024-04-03 at 17 37 20

ISSUE 10 🆕
This is linked issue 2 but documenting it here so that it's more clear-cut.
While we don't have figma for mobile/tablet, I do want to double check on why we have omitted the Title on those breakpoints.
I think the title is an important piece of information that should not get hidden on smaller screen.
Happy to be overruled though.

Image are attached

Screenshots

Currently on mobile/tablet viewport, the Title 'New visitors' and the tooltip get hidden

Screenshot 2024-04-03 at 17 42 44

Reference for desktop view

Screenshot 2024-04-03 at 16 59 37

@benbowler
Copy link
Collaborator

Thanks @kelvinballoo, updates below:

ISSUE 2 : ❌: You can see the tooltip version of the story here.

ISSUE 9 ❌: I made the margin bottom 16px in total rather than adding an additional 16px. PR connected to this ticket with the change and will move to CR once checks pass.

@benbowler benbowler assigned tofumatt and unassigned benbowler and tofumatt Apr 3, 2024
@tofumatt tofumatt assigned kelvinballoo and unassigned tofumatt Apr 3, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ⚠️

ISSUE 2 is fine ✅ . Spotted the tooltip on the Storybook URL given.

Screenshot

Size (16x16) and margin of 5 px are as expected

Screenshot 2024-04-04 at 13 39 39
__________________________________________________________________________________________

ISSUE 9 is fine ✅ . The extra 16px margin has been implemented.

Screenshot Screenshot 2024-04-04 at 13 40 55
__________________________________________________________________________________________

ISSUE 10 ⚠️
Need further review on this.

While we don't have figma for mobile/tablet, I do want to double check on why we have omitted the Title on those breakpoints.
I think the title is an important piece of information that should not get hidden on smaller screen.
Happy to be overruled though.

@benbowler , this might have been overlooked. Let me know your thoughts.
If there is no concern, we can mark this as pass.
Else, if this needs further discussion with UX/UI team, happy for this to be moved to another ticket/thread.

@benbowler
Copy link
Collaborator

Hey @kelvinballoo, the reason the title is hidden on smaller breakpoints is that, in these breakpoints, the AudienceTile component will be loaded in a tabbed layout by the parent AudienceTiles component. These tabs will show the title of each tile in the tabs themselves, hence we can hide the title in the child component otherwise the title will be duplicated in the tabs and the tile itself. Does that make sense?

@benbowler benbowler assigned kelvinballoo and unassigned benbowler Apr 4, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

Thanks for the clarification @benbowler !
That makes sense 💯

Moving this to 'Approval'

@kelvinballoo kelvinballoo removed their assignment Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers 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