site-kit-wp
site-kit-wp copied to clipboard
Add the Audience Tiles widget (Storybook)
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, sayconfiguredAudiences
, which is implemented together with selector above in 8176 - [ ] Make a report request using
getReport
action onMODULES_ANALYTICS_4
datastore. You can see an example in any of the key metrics widgets on how to formatreportOptions
and handlegetReport
. ThereportOptions
object should:- [x] Include
audienceResourceName
in dimensions - [x] Add
dimensionFilters
foraudienceResourceName
dimension, withinListFilter
type, and pass theconfiguredAudiences
array which will hold the audiences name fields, which translate toaudienceResourceName
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 withoutaudienceResourceName
dimension included. And although not mentioned in design doc, potentiallyTop content by pageviews
might need separate handling due to thepagePaths
dimension which will needgetPageTitles
action to match the titles with paths, and also usage of custom dimensions. Refer to thePopularContentWidget
component for usage example on titles.
- [x] Include
- Loading and errors are handled in separate issues, as well as placeholder tile
- [x] Wrap the rendered output with
Widget
component, and add a classgooglesitekit-widget__audience-tiles
to it. - [ ] Use
useBreakpoint
hook to extract the current device size- [ ] In case of
BREAKPOINT_SMALL
, usegetAudiences()
selector to retrieve audiences, filtered to match only the ones inconfiguredAudiences
.- [ ] You can use the
name
property ofgetAudiences
for filtering. - [ ] Use
displayName
from results to collect the tiles titles and rendered them in separatediv
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
- [ ] You can use the
- [ ] In case of
- [ ] Loop through
configuredAudiences
:- [ ] Filter the report response data for
audienceResourceName
dimension, and prepare the values to match the required/expected props inAudienceTile
implemented in 8135 - [ ] Render
AudienceTile
component
- [ ] Filter the report response data for
- [x] It should accept
- [ ] 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
- [ ] Responsive breakpoints should follow the guidelines under
Test Coverage
- Add stories and jest tests for
AudienceTiles
component
QA Brief
Changelog entry
- 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.
Thanks @eugene-manuilov . Let me know if it's clearer now. Thanks!
Yes, thanks! AC ✔️
IB ✔️
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 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?
@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 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.
Thanks @benbowler. Good shout, #8484 looks like an appropriate issue to tackle this in. Please do go ahead as you have suggested. Cheers!
Thanks @techanvil, I've added a comment to #8484, as well as inline comments, and moved this ticket back into review.
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.
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.
Implementation: The icons are closer to the top
ITEM 2: The 'U' is 'All Users' should be small caps, to keep it similar and consistent with other tabs.
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.
ITEM 4: Is the section 'Understand how different visitor groups interact with your site' supposed to be part of the widget/component?
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.
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 :)
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.
That's great, thanks @kuasha420.
Back to you for another pass, @kelvinballoo!
QA Update :warning:
Thanks @kuasha420 , @techanvil
Retested good for items 1 and 5.
ITEM 1 RETEST The icons are now vertically centred. ✅

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

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?
Thanks @kelvinballoo. The ellipses will be covered in the followup issue https://github.com/google/site-kit-wp/issues/8626.
QA Update ✅
Sounds good @techanvil . Since there is no other outstanding item, I am moving this ticket to Approval.