Add the Audience Tile happy path view (Storybook)

techanvil commented 9 months ago

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

[X] Add assets/js/modules/analytics-4/components/widgets/AudienceSegmentation/AudienceTile.js
- Include the following props:
- title
- infoTooltip
- visitors
- visitsPerVisitors
- pagesPerVisit
- pageviews
- topCities
- topContent
- Widget
[x] 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.
[x] 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
[x] Wrap the component output with <Widget noPadding>, and a main child div with class, say googlesitekit-audience-segmentation-tile
[x] Borrow the logic for infoTooltip components from one of the key metrics widgets
[x] Tile title and info tooltip should be grouped under same div wrapper
[x] 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
[x] 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
[x] 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.

eugene-manuilov commented 9 months ago

@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 commented 9 months ago

@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 commented 9 months ago

@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 commented 9 months ago

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.

asvinb commented 9 months ago

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

eugene-manuilov commented 9 months ago

Yes, thanks, Asvin. AC ✔️

eugene-manuilov commented 8 months ago

IB ✔️

kelvinballoo commented 7 months ago

QA Update ❌

Tests were done on Dev environment and Chrome Desktop on MacOS Ventura. Since there is quite a number of feedback on desktop, I prefer to get those ironed out first before moving to cross browser and cross device checks. Tested URL: https://google.github.io/site-kit-wp/storybook/develop/?path=/story/modules-analytics4-components-audiencesegmentation-audiencetile--ready Figma reference: https://www.figma.com/file/7pSrkEy8t00BcYRAi9LjjH/Audience-Segmentation?node-id=1046%3A42107&mode=dev

Issues and screenshots are detailed below:

ISSUE 1 The overall tile margin on the side should be 24px and not 16px.

Screenshots

Figma: 24px

Storybook implementation: 16 px

ISSUE 2 Title of tile and tooltip are missing along with the line below it.

Screenshots

Figma: Highlighted all the needed elements

Storybook implementation: Elements missing

ISSUE 3 Overall height of a section should be 76px and not 80px

Screenshots

Figma: 76 px in height

Storybook implementation: 80 px (56+12+12)

ISSUE 4 Right margin to the right of the icon should be 20px. It's currently 19px which is rather odd

Screenshots

Figma: 20px

Storybook implementation: 19px

ISSUE 5 Icon is not centrally aligned vertically

Screenshots

Figma: Both arrows are the same size and icon is vertically centred

Storybook implementation: The arrows are of the same size but there is a white gap, meaning the icon is not centrally aligned on the verical

ISSUE 6 There should be a margin gap before the start of the icon.

Screenshots

Figma: 16px gap with the start of the grey line

Storybook implementation: No gap implemented

ISSUE 7 For the page view stats, it's in the K format on storybook but in figma, it's the full number e.g. 1,565 And for the %, should we remove the decimal place?"

Screenshots

Figma: Full number format: 1565 and percentage is without decimal place

Storybook implementation: No gap implemented

ISSUE 8 The space below 'Cities with the most visitors' is 14 px but it should be 12px Column gap should be 10px instead of 16px. There should be a 2px gap between the city and the %

Screenshots

Storybook implementation:

ISSUE 9 There are multiple items to highlight under the last section. Better refer to the screenshot

Screenshot

Storybook implementation:

benbowler commented 7 months ago

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.

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 commented 7 months ago

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

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

Figma

ISSUE 3 ✅ The sections are now 76px.

Screenshot

52+12+12 = 76 px

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

Screenshot

20px on the right of the icon

ISSUE 5 ✅ The icon is not vertically centred.

Screenshot

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

Screenshot

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

Implementation: only 12 (+4 from the text)

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

Reference for desktop view

benbowler commented 7 months ago

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.

kelvinballoo commented 7 months ago

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

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

Screenshot

ISSUE 10 :warning: 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 commented 7 months ago

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?

kelvinballoo commented 7 months ago

QA Update ✅

Thanks for the clarification @benbowler ! That makes sense 💯

Moving this to 'Approval'

google / site-kit-wp