site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Add the Audience Tiles widget (Storybook)

Open techanvil opened this issue 1 year ago • 5 comments

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

  • [x] Add Add assets/js/modules/analytics-4/components/widgets/AudienceSegmentation/AudienceTiles.js
    • [x] It should accept Widget prop
    • [x] Add local state for managing the currently active tile, eg. activeTile, setActiveTile. This will be used to mobile view to determine the visible tile.
    • [x] 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:
      • [x] Include audienceResourceName in dimensions
      • [x] 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 https://github.com/google/site-kit-wp/blob/ab9d76bebe8a3368eaf73b194edf941fd42f2ec5/assets/js/modules/analytics-4/components/widgets/TopRecentTrendingPagesWidget.js#L121-L130
      • [ ] 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
    • [x] 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

Changelog entry

techanvil avatar Jan 24 '24 14:01 techanvil

  • 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.

eugene-manuilov avatar Feb 01 '24 13:02 eugene-manuilov

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

asvinb avatar Feb 08 '24 12:02 asvinb

Yes, thanks! AC ✔️

eugene-manuilov avatar Feb 08 '24 14:02 eugene-manuilov

IB ✔️

eugene-manuilov avatar Feb 13 '24 18:02 eugene-manuilov

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.

techanvil avatar Mar 20 '24 15:03 techanvil

@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 https://github.com/google/site-kit-wp/pull/8395#discussion_r1530308111 on https://github.com/google/site-kit-wp/issues/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 avatar Apr 11 '24 10:04 benbowler

@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?

Thanks @benbowler. I'd agree that things can be simplified by modifying the prop structure of the AudienceTile component. I think it's a bit of an unnecessary abstraction the way it stands and it would make more sense to simply pass the reports directly in as props.

I realise it was a bit hard to figure this out in advance hence the current implementation, but I feel it could well be appropriate to add a followup issue to refactor the props as per the above. This should also allow us to make the stories more consistent as we'd simply need to provide the same sort of reports in both cases without much or any additional data manipulation.

I realise @zutigrm has made some suggestions for refactoring the data extraction which could improve things in this iteration (noting the comments https://github.com/google/site-kit-wp/pull/8497/files#r1565586992 and https://github.com/google/site-kit-wp/pull/8497/files#r1565590237), so let's take a look once you've addressed these and proceed with creating a followup issue for a refactor if it still seems appropriate; also, it might be worth simply not making these updates with the proposed refactor in mind.

techanvil avatar Apr 15 '24 11:04 techanvil

@techanvil I think #8484 is the best ticket to implement both the changes to the AudienceTile props and updating the AudienceTile stories to use the data-mock function, as any changes made to this iteration will need to be removed at that point anyway.

If you agree, I will add an additional inline comment and leave a comment on #8484 to ensure changes are made in that ticket.

benbowler avatar Apr 16 '24 14:04 benbowler

Thanks @benbowler. Good shout, #8484 looks like an appropriate issue to tackle this in. Please do go ahead as you have suggested. Cheers!

techanvil avatar Apr 16 '24 15:04 techanvil

Thanks @techanvil, I've added a comment to #8484, as well as inline comments, and moved this ticket back into review.

benbowler avatar Apr 16 '24 16:04 benbowler

Thanks @benbowler. As mentioned on Slack this should go back to @zutigrm to finish his review, so I've assigned it back to him. Although, if he's off tomorrow I'd be happy to pick it up myself.

techanvil avatar Apr 16 '24 17:04 techanvil

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

kelvinballoo avatar Apr 24 '24 08:04 kelvinballoo

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 https://github.com/google/site-kit-wp/issues/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 https://github.com/google/site-kit-wp/issues/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 https://github.com/google/site-kit-wp/issues/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 https://github.com/google/site-kit-wp/issues/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 :)

techanvil avatar Apr 24 '24 10:04 techanvil

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 avatar Apr 26 '24 12:04 kuasha420

That's great, thanks @kuasha420.

Back to you for another pass, @kelvinballoo!

techanvil avatar Apr 26 '24 15:04 techanvil

QA Update :warning:

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?

kelvinballoo avatar Apr 26 '24 16:04 kelvinballoo

Thanks @kelvinballoo. The ellipses will be covered in the followup issue https://github.com/google/site-kit-wp/issues/8626.

techanvil avatar Apr 26 '24 16:04 techanvil

QA Update ✅

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

kelvinballoo avatar Apr 28 '24 09:04 kelvinballoo