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 Introductory Popup / Banner (Storybook) #8171
Comments
IB ✅ |
Thanks @kelvinballoo. I'll try to address your observations. I'll address whatever needs addressing in a follow up PR.
Thank you! |
Hey @kuasha420, reviewing the points in your reply above:
Tbh, I am actually a bit confused here - I'm not clear on what the additional context should be taken from the linked thread. Furthermore though, in theory we shouldn't be integrating this popup/banner into Site Kit as part of this issue. The component is supposed to be shown conditionally upon setting up the Audience Segmentation feature, just adding it to Storybook in this issue and then integrating it via #8172. Still, I suppose it's behind the feature flag and so it doesn't really hurt to show it unconditionally for now so we can verify it works as expected when integrated, and we can simply apply the relevant conditions as part of 8172. Anyhow, @kuasha420, please can you double check the linked thread for relevancy and add a bit of further clarification for this point?
Your replies to points 2 through 6 LGTM.
As you've pointed out, we should be aiming for consistency within the plugin. Sometimes Figma is a bit out of alignment with what's already there - it's something we should certainly aim to improve going forward but in the meantime we do have to use our judgement to assess what the concrete implementation should look like. In this case, it's clear the margins are consistent with other similar banners e.g. the So, to summarise point 7 I would say the implementation LGTM as it stands. |
@techanvil Thank you for the response. I made a mistake in linking the slack message. This is the right one. Oops. cc @kelvinballoo |
@techanvil @kelvinballoo The first point of ITEM 6 regarding the size is also fine I think, it maintains the aspect ratio and and changes size based on screen size on mobile, so it will not be exactly same as Figma and most other SVGs in the plugin behaves that way. With that said, the follow up PR that fixes the overlay position on tablet/desktop (bottom/right margin) and the whitespace under the SVG on mobile is now in CR. Cheers |
Thanks @kuasha420. The followup PR is merged, back to you for another look, @kelvinballoo. |
QA Update ✅Thanks @techanvil , @kuasha420 . Noted on all the points clarified. Tested the outstanding items as follows:
These are looking good. |
Feature Description
Create the Introductory Popup / Banner component and add it to Storybook.
Note that this component can build upon that introduced in either #8236 or #8237. Both of these have been added as dependencies, but this issue can be implemented when one of those is merged, whichever comes first.
See introductory popup / banner in the design doc.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
The tablet design should use the graphic variant with three groups as seen in the tablet design for the Setup CTA Banner.Implementation Brief
assets/js/components/OverlayNotification/AudienceSegmentationIntroductoryOverlayNotification.js
assets/js/components/OverlayNotification/LinkAnalyticsAndAdSenseAccountsOverlayNotification.js
or any other overlay notification as an exampleNew! Visitor groups
You can now learn more about your site visitor groups by comparing different metrics
Got it
CTA withdismissNotification
action like currently used in other overlay notifications.useBreakpoint
hook, and check the device width to determine which graphic to render. Follow the figma as pointed out in AC, to render appropriate graphics onBREAKPOINT_DESKTOP
and up,BREAKPOINT_TABLET
and less up to theBREAKPOINT_SMALL
, and forBREAKPOINT_SMALL
.AUDIENCE_SEGMENTATION_INTRODUCTORY_OVERLAY_NOTIFICATION
constant and pass it to thesetOverlayNotificationToShow
in theuseEffect
hook. Currently it should be shown with only one conditional check -if it has not been dismissedisShowingNotification
variable to check forisShowingOverlayNotification
only if the breakpoint is equal or higher than desktop, otherwise it should only check if notification is not dismisseddiv.googlesitekit-audience-segmentation-notification
AudienceSegmentation
feature flag is not enabled to return earlyassets/sass/components/overlay-notification/_googlesitekit-audience-segmentation-notification.scss
googlesitekit-audience-segmentation-notification
class to override the styles, currently what I saw the font weight and letter-spacing styles of introductory popup body text differs from the ones used in overlay notification. And any other differences as per figma design linked in ACfixed
positionassets/js/components/OverlayNotification/OverlayNotificationsRenderer.js
for breakpoints less thenBREAKPOINT_DESKTOP
and inassets/js/components/Header.js
for breakpoints equal or greater thanBREAKPOINT_DESKTOP
Test Coverage
AudienceSegmentationIntroductoryOverlayNotification
componentQA Brief
audienceSegmentation
feature flag.Changelog entry
The text was updated successfully, but these errors were encountered: