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 Tiles widget (Storybook) #8136

Closed
21 tasks done
techanvil opened this issue Jan 24, 2024 · 18 comments
Closed
21 tasks done

Add the Audience Tiles widget (Storybook) #8136

techanvil opened this issue Jan 24, 2024 · 18 comments
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jan 24, 2024

Feature Description

Create the Audience Tiles widget component and add it to Storybook, showing the happy path audience tiles.

Note that this widget is displayed in a tabbed view for the mobile viewport while the desktop view is displayed as a list of tiles, and as a scrollable list at narrower desktop viewports when there are three tiles.

See audiences 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 tiles widget.
  • The UI should be similar to the Figma designs and necessary props should be added to the component to achieve it.
  • 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.
    • The component should be able to render one or more tiles title on phones/tablets.

Implementation Brief

  • Add Add assets/js/modules/analytics-4/components/widgets/AudienceSegmentation/AudienceTiles.js
    • It should accept Widget prop
    • Add local state for managing the currently active tile, eg. activeTile, setActiveTile. This will be used to mobile view to determine the visible tile.
    • Use getConfiguredAudiences() selector to retrieve configured audiences and save it in a variable, say configuredAudiences, which is implemented together with selector above in 8176
    • Make a report request using getReport action on MODULES_ANALYTICS_4 datastore. You can see an example in any of the key metrics widgets on how to format reportOptions and handle getReport. The reportOptions object should:
      • Include audienceResourceName in dimensions
      • Add dimensionFilters for audienceResourceName dimension, with inListFilter type, and pass the configuredAudiences array which will hold the audiences name fields, which translate to audienceResourceName dimension. For an example on how to use this filter you can refer to this code
        dimensionFilters: {
        'customEvent:googlesitekit_post_date': {
        filterType: 'inListFilter',
        value: [
        yesterday.replace( /-/g, '' ),
        twoDaysAgo.replace( /-/g, '' ),
        threeDaysAgo.replace( /-/g, '' ),
        ],
        },
        },
      • Include the metrics and dimensions from the list, defined under Metric definitions in the design doc
      • Note that as pointed out in the design doc - Pageviews should be acquired in separate report request without audienceResourceName dimension included. And although not mentioned in design doc, potentially Top content by pageviews might need separate handling due to the pagePaths dimension which will need getPageTitles action to match the titles with paths, and also usage of custom dimensions. Refer to the PopularContentWidget component for usage example on titles.
    • Loading and errors are handled in separate issues, as well as placeholder tile
    • Wrap the rendered output with Widget component, and add a class googlesitekit-widget__audience-tiles to it.
    • Use useBreakpoint hook to extract the current device size
      • In case of BREAKPOINT_SMALL, use getAudiences() selector to retrieve audiences, filtered to match only the ones in configuredAudiences.
        • You can use the name property of getAudiences for filtering.
        • Use displayName from results to collect the tiles titles and rendered them in separate div wrapper above the tiles.
        • Make use of activeTile to switch between tiles and conditionally render equivalent tile.
        • They should be styled as tabs, per figma designs
    • Loop through configuredAudiences :
      • Filter the report response data for audienceResourceName dimension, and prepare the values to match the required/expected props in AudienceTile implemented in 8135
      • Render AudienceTile component
  • Add assets/sass/components/analytics-4/audience-segmentation/audience-tiles.scss and apply the styles to match the figma designs linked in AC for desktop and mobile.
    • Responsive breakpoints should follow the guidelines under Responsive behavior section of the design doc

Test Coverage

  • Add stories and jest tests for AudienceTiles component

QA Brief

  • View the storybook story for the AudienceTiles component, confirming that it meets the AC and matches the figma mocks.

Changelog entry

  • Add the Audience Tiles widget as a component 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

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

@asvinb, this one is similar to #8135. We need to re-purpose AC to focus on the widget component creation as it is the main goal here. A new storybook story is just an addition to the new component to be able to see how it works since it won't be connected to the codebase yet.

@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! AC ✔️

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

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Feb 13, 2024
@techanvil
Copy link
Collaborator Author

Note: As discussed on #8135, when implementing this issue we should consider updating the AudienceTile story to use generated mock report data to replace the hardwired data that it was initially implemented with.

@benbowler
Copy link
Collaborator

@techanvil I have an implementation in the attached PR, I've added details into the PR technical description. One thing I noted is that the report structure didn't really match the prop structure defined in the AudienceTile component which leads to a lot of restructuring here in the AudienceTiles component. We will have to do some form of restructuring however I think we could simplify this somewhat by adjusting the prop structure of the AudienceTile.


Note: As #8395 (comment) on #8135, when implementing this issue we should consider updating the AudienceTile story to use generated mock report data to replace the hardwired data that it was initially implemented with.

As the AudienceTile component doesn't itself contain the selectors for the data, when looking into this, I ended up reimplementing AudienceTiles structure inside of the AudieceTile stories which was super messy IMO. Perhaps there is a middle ground or the boilerplate is worth it here to keep the stories setup consistent across stories?

@benbowler benbowler removed their assignment Apr 11, 2024
@kuasha420 kuasha420 assigned techanvil and unassigned kuasha420 Apr 23, 2024
@techanvil techanvil removed their assignment Apr 23, 2024
@kelvinballoo kelvinballoo self-assigned this Apr 24, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ❌

Hi @kuasha420 , this is looking good overall.
I need some clarification/guidance and raised a few points below for you to review if fixes are required:

ITEM 1:
The icons for the last two sections are currently not vertically centred.
They look closer to the top of the section. Per figma, they should be centred vertically.

Figma: The arrows are the same size, which indicates equal distances top and bottom. On the last section, there is an extra space because of the margin of the entire card.
Screenshot 2024-04-24 at 14 42 17

Implementation: The icons are closer to the top
Screenshot 2024-04-24 at 14 41 54

________________________________________________________________________

ITEM 2:
The 'U' is 'All Users' should be small caps, to keep it similar and consistent with other tabs.

Screenshot 2024-04-24 at 14 48 08
________________________________________________________________________

ITEM 3:
Do we have to implement the 'Partial Data' under this ticket or another one? If another it's fine. (Refer to screenshots below)
Also, can we add a sample data to ensure we have captured the ellipsis for long content? Screenshot below.

Ellipsis and partial data highlighted: Screenshot 2024-04-24 at 14 50 09
________________________________________________________________________

ITEM 4:
Is the section 'Understand how different visitor groups interact with your site' supposed to be part of the widget/component?

Screenshot 2024-04-24 at 14 58 49
________________________________________________________________________

ITEM 5:
Do we have a variation for 1 tile or 2 tiles?
I just wanted to review the following parts from the PRD:
At 961px and above, if there are two tiles they will be displayed in a list, each taking 50% of the available space.
Where only one audience tile is present, a placeholder tile will also be displayed, as we don’t want the single tile to expand to the full page width; note that this is only applicable to the desktop view,
If those are outside of the scope for this ticket, we can ignore that for now.


ITEM 6:
Currently the last 2 sections of all 3 tabs point to the same data.
Any chance we could update each data for each tab differently so that we know it's dynamic?
Again if this lies out of scope whereby these are just sample data without any real backend implication, then let's ignore.

Screenshot 2024-04-24 at 15 06 29

@techanvil techanvil self-assigned this Apr 24, 2024
@techanvil
Copy link
Collaborator Author

Hi @kelvinballoo, thanks for raising these points. I'm going to answer these as I've got more context on the issue and epic as a whole. I'll then let @kuasha420 create a followup PR to address what does need addressing here.

ITEM 1: The icons for the last two sections are currently not vertically centred. They look closer to the top of the section. Per figma, they should be centred vertically.

Good spot - this should indeed be updated to vertically centre the icons as per Figma.

ITEM 2: The 'U' is 'All Users' should be small caps, to keep it similar and consistent with other tabs.

This doesn't need addressing here. For a bit of context - we'll mostly simply be displaying the audiences names as they are set in Analytics - the capitalisation will therefore be up to the user's preferences for the most part.

That said, "All Users" is a special case as one of the default audiences. Because we are going to be creating "New visitors" and "Returning visitors" with this capitalisation we do actually want to change the way we display the "All Users" audience for consistency, so your point is still valid.

However, we are going to be addressing this in a separate issue where we map "All Users" to "All visitors" - see #8487.

ITEM 3: Do we have to implement the 'Partial Data' under this ticket or another one? If another it's fine. (Refer to screenshots below) Also, can we add a sample data to ensure we have captured the ellipsis for long content? Screenshot below.

The "partial data" badges will be implemented via a separate issue, see #8142.

Good shout about ensuring we capture the ellipses in Storybook though, this is worth an update here.

ITEM 4: Is the section 'Understand how different visitor groups interact with your site' supposed to be part of the widget/component?

No, this will be addressed via #8138.

ITEM 5: Do we have a variation for 1 tile or 2 tiles? I just wanted to review the following parts from the PRD: At 961px and above, if there are two tiles they will be displayed in a list, each taking 50% of the available space. Where only one audience tile is present, a placeholder tile will also be displayed, as we don’t want the single tile to expand to the full page width; note that this is only applicable to the desktop view, If those are outside of the scope for this ticket, we can ignore that for now.

Adding a two-tile story is a good shout and can be addressed here.

However please note that the placeholder tiles will be covered in a separate issue, see #8146.

ITEM 6: Currently the last 2 sections of all 3 tabs point to the same data. Any chance we could update each data for each tab differently so that we know it's dynamic? Again if this lies out of scope whereby these are just sample data without any real backend implication, then let's ignore.

This doesn't really need addressing as it's just sample data; that said it could be worth showing different data to help illustrate the fact that they are indeed using separate reports. If it's not too much effort I'd say it's worth addressing this here too.

Ok, over to you @kuasha420, please send it back to me for the followup CR :)

@kuasha420
Copy link
Contributor

Thank you @kelvinballoo & @techanvil !

Based on your feedback, I've filed a follow up PR that fixes ITEM 1 and 5.

Item 3 will be fixed via #8626 and ITEM 6 will be fixed by #8627

Thank you.

@kuasha420 kuasha420 assigned techanvil and unassigned kuasha420 Apr 26, 2024
@techanvil
Copy link
Collaborator Author

That's great, thanks @kuasha420.

Back to you for another pass, @kelvinballoo!

@techanvil techanvil assigned kelvinballoo and unassigned techanvil Apr 26, 2024
@kelvinballoo
Copy link
Collaborator

kelvinballoo commented Apr 26, 2024

QA Update ⚠️

Thanks @kuasha420 , @techanvil

Retested good for items 1 and 5.

ITEM 1 RETEST
The icons are now vertically centred. ✅

Screenshot 2024-04-27 at 00.02.19.png
____________________________________________________________

ITEM 5 RETEST:
There is now a 2 card layout and the cards expand 50:50 as we move from 961px wide and beyond. ✅

Screenshot 2024-04-27 at 00.06.23.png
____________________________________________________________

Understand that item 2, 3, 4 and 6 will be addressed in other tickets.

That said, I understood that the Ellipsis part under item 3 would be captured in this ticket.
Do we want to include it or address it in another ticket as well?

@techanvil
Copy link
Collaborator Author

Thanks @kelvinballoo. The ellipses will be covered in the followup issue #8626.

@techanvil techanvil assigned kelvinballoo and unassigned kuasha420 and techanvil Apr 26, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

Sounds good @techanvil .
Since there is no other outstanding item, I am moving this ticket to Approval.

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

No branches or pull requests

10 participants