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

Adjust the CTA and "New" badge positions for widget areas on mobile and tablet viewports. #8863

Closed
13 tasks
kelvinballoo opened this issue Jun 12, 2024 · 10 comments
Closed
13 tasks
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@kelvinballoo
Copy link
Collaborator

kelvinballoo commented Jun 12, 2024

Bug Description

The 'Change groups' should appear at the bottom of the text Understand how different visitor groups interact with your site on mobile and tablet.
Currently, it appears at the top.

Steps to reproduce

  1. Turn on audience segmentation featured flag
  2. View the SK dashboard on mobile and tablet
  3. Notice where the 'Change groups' CTA is located.

Screenshots

Implementation:
Screenshot 2024-06-11 at 19 17 43

Figma:
Screenshot 2024-06-11 at 19 18 16

Additional Context

  • PHP Version: 8.0
  • Browser: Chrome
  • Plugin Version: 1.129.0
  • Device: MacOS Sonoma on MacbookPro

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Widget area CTA position

  • For viewports up to and inclusive of 782px, the CTA for a widget area should appear below the area and start-aligned.
  • For viewports from 783px, the CTA for a widget area should appear above the area and end-aligned, as it is now.
  • In concrete terms this should apply to the Audience Segmentation and Key Metrics widget area CTAs.
  • See the relevant Figma designs for Audience Segmentation:

Widget area "New" badge position

  • For viewports up to and inclusive of 600px, the "New" badge for a widget area should be end-aligned.
  • For viewports from 601px, the "New" badge should follow the document flow and be near the title text, as it is now.
  • In concrete terms this should apply to the Audience Segmentation widget area.
  • See relevant Figma designs:

Implementation Brief

  • Update assets/js/googlesitekit/widgets/components/WidgetAreaRenderer.js:
    • Use the useWindowWidth hook, if the window width is >= 783px render the CTA component in the widget area header.
      { CTA && (
      <div className="googlesitekit-widget-area-header__cta">
      <CTA />
      </div>
      ) }
    • If the window width is <= 782px, render the CTA within a new Cell in the footer Row component. Update the conditional block to make sure this row renders the CTA here regardless of if there is a Footer set but to only render the Footer if it exists.
      • Note that due to the existing nature of the footer Cell component, set to full width (grid size 12), conditional cell widths for large, medium and small viewports will need to be provided. These values should be half the total available grid count for the viewport in question, so as to maintain equidistant spacing for the CTA and Footer elements.
        { Footer && (
        <Row>
        <Cell
        className="googlesitekit-widget-area-footer"
        size={ 12 }
        >
        <Footer />
        </Cell>
        </Row>
        ) }
    • Give the footer CTA the class googlesitekit-widget-area-footer__cta and update the CSS to match designs in assets/sass/widgets/_widget-area.scss.
      • Provide a fixed height to the new bade component, using existing computed height as a reference, so that it does not adapt to the height of the flexbox in which it is being rendered.
  • Update assets/sass/widgets/_widget-area.scss:
    • Update .googlesitekit-widget-area-header__subtitle, setting: flex-grow: 1, display: flex and justify-content: space-between;.
    • Add a new media query for min-width: $bp-tablet, setting display: block to remove the space between flex on larger screens.
    • Separate the new badge logic into it's own standalone component to avoid an eslint complexity violation
      • Create a new component, WidgetNewBadge, accepting a widget slug prop (required)
      • Port over the existing new badge rendering logic from WidgetAreaRenderer component into said new component
      • Import and make use of said component as a replacement to the conditional NewBadge logic currently in the WidgetAreaRenderer component

Test Coverage

  • Update VRTs for both the Audience Segmentation and Key Metrics widget areas, confirming there are no unexpected visual or functional regressions.

QA Brief

  • Setup Site Kit with GA4 module and audienceSegmentation feature flag enabled
  • On desktop:
    • Navigate to the Audiences area of the main dashboard and validate the following:
      • The title, new badge and CTA (change groups) remains unaffected (screenshot below)
        Site Kit by Google Dashboard ‹ sitekit 10uplabs com — WordPress 2024-08-12 at 12 25 06 PM
  • On tablet to mobile:
    • Navigate to the Audiences area of the main dashboard and validate the following:
      • The CTA (change groups) no longer appears in the header, but the new badge remains in the header (screenshot below):
        Site Kit by Google Dashboard ‹ sitekit 10uplabs com — WordPress 2024-08-12 at 12 41 04 PM
      • The CTA has moved to the footer area, as per designs, and footer elements remain to the right as per current functionality (screenshot below):
        Site Kit by Google Dashboard ‹ sitekit 10uplabs com — WordPress 2024-08-12 at 12 41 26 PM

Changelog entry

  • Adjust the CTA and "New" badge positions for widget areas on mobile and tablet viewports.
@nfmohit
Copy link
Collaborator

nfmohit commented Jun 13, 2024

This is something that I think needs a little confirmation from @sigal-teller and @techanvil. The CTA button here is attached to the widget area. We can make it show up in the footer of the widget area for mobile breakpoints (this will also apply globally and will impact the KM "Change metrics" CTA as well), but the Audience Segmentation widget area also has an additional InfoNoticeWidget underneath the audience tiles, so the "Change groups" CTA will appear below the InfoNoticeWidget. Is it okay to show it there, or do we want to do something else here?

Here's a mock of what it may look:
image

@techanvil techanvil self-assigned this Jun 13, 2024
@techanvil
Copy link
Collaborator

Thanks @kelvinballoo and @nfmohit.

Showing the CTA below the widget area as per the mockup above looks fine to me. We do have a design in Figma showing the CTA on the same line as the source link:

image

Applying it across the board, i.e. to the KM widget area as well also makes sense - here's how it currently looks at a narrow mobile viewport:

image

Here's a mockup showing how it would look with the CTA below the widget area:

image

As I've said it's all LGTM, but it would be good to get @sigal-teller's thoughts too.

@sigal-teller
Copy link
Collaborator

@techanvil Placing it below the widget LGTM (if we'll have the insight notice it will be placed below like it was mentioned here). In KMW it should also appear below the widget, per Figma design. Thanks!

@techanvil
Copy link
Collaborator

techanvil commented Jun 13, 2024

Thanks @sigal-teller.

Just noting that, as per the Figma design, we'll show the CTA below the widget area up to a breakpoint of 782px, from 783px we'll show it above the widget area (see related comment thread).

@techanvil techanvil assigned techanvil and unassigned techanvil and sigal-teller Jun 13, 2024
@techanvil techanvil changed the title The 'Change groups' should appear at the bottom on mobile and tablet. The 'Change groups' should appear at the bottom on mobile and tablet viewports up to 800px Jun 13, 2024
@techanvil techanvil assigned techanvil and unassigned techanvil Jun 13, 2024
@techanvil techanvil added P1 Medium priority Type: Enhancement Improvement of an existing feature Module: Analytics Google Analytics module related issues labels Jun 13, 2024
@techanvil techanvil changed the title The 'Change groups' should appear at the bottom on mobile and tablet viewports up to 800px The 'Change groups' should appear at the bottom on mobile and tablet viewports up to 783px Jun 13, 2024
@techanvil techanvil removed their assignment Jun 13, 2024
@techanvil techanvil changed the title The 'Change groups' should appear at the bottom on mobile and tablet viewports up to 783px The 'Change groups' CTA should appear below the widget area on mobile and tablet viewports up to 783px Jun 14, 2024
@ivonac4 ivonac4 added the Team M Issues for Squad 2 label Jun 17, 2024
@techanvil techanvil self-assigned this Jun 19, 2024
@techanvil techanvil changed the title The 'Change groups' CTA should appear below the widget area on mobile and tablet viewports up to 783px Adjust the CTA and "New" badge positions for widget areas on mobile and tablet viewports. Jun 20, 2024
@techanvil
Copy link
Collaborator

Noting that as discussed on Figma, we'll include the alignment of the "New" badge in this issue as well as the widget area CTA.

@techanvil techanvil removed their assignment Jun 20, 2024
@benbowler benbowler assigned benbowler and unassigned benbowler Jun 24, 2024
@nfmohit nfmohit self-assigned this Jun 24, 2024
@nfmohit
Copy link
Collaborator

nfmohit commented Jun 24, 2024

Nice work on the IB here, @benbowler! Please look at my comments below:

  • Add the 783px breakpoint to assets/js/hooks/useBreakpoint.js as BREAKPOINT_WP_ADMIN_BAR_TABLET = 'wpadminbar'

I'm not sure we want to do this. For example, currently for a viewport of 784 pixels, useBreakpoint returns BREAKPOINT_TABLET which is what a lot of the usages of this function would expect. With the proposed change, the function would return BREAKPOINT_WP_ADMIN_BAR_TABLET for a viewport of 784 pixels, thus potentially breaking the existing usages.

Instead, we could do something like what we're doing in assets/js/components/FeatureToursDesktop.js and directly use useWindowWidth as an exception. What do you think?

  • Use the useBreakpoint hook, if the breakpoint is ! BREAKPOINT_TABLET && ! BREAKPOINT_SMALL, do not render the CTA component in the widget area header.

If we do go with directly using useWindowWidth as explained above, let's simplify this to just render the CTA component in the widget area header if the window width is 783 pixels or more.

  • If the breakpoint is BREAKPOINT_TABLET || BREAKPOINT_SMALL, render the CTA within a new Cell in the footer Row component. Update the conditional block to make sure this row renders regardless of if there is a Footer set but to only render the Footer if it exists.

If we do go with directly using useWindowWidth as explained above, let's simplify this to just render the CTA component in the widget area footer if the window width is 782 pixels or less.

  • Update assets/sass/widgets/_widget-area.scss, add a new media query up to 600px using $bp-mobileOnly:

Let's propose a mobile-first approach here, so that the suggested styles are added as default, and overridden on viewports with a min-width of $bp-nonMobile.

Please let me know what you think, thank you!

@nfmohit nfmohit assigned benbowler and unassigned nfmohit Jun 24, 2024
@benbowler benbowler assigned nfmohit and unassigned benbowler Jun 26, 2024
@nfmohit
Copy link
Collaborator

nfmohit commented Jun 26, 2024

IB ✅ Thanks @benbowler !

@nfmohit nfmohit removed their assignment Jun 26, 2024
@binnieshah binnieshah added Team S Issues for Squad 1 and removed Team M Issues for Squad 2 labels Jul 23, 2024
@10upsimon 10upsimon self-assigned this Jul 24, 2024
@10upsimon
Copy link
Collaborator

Noting as per communication with @techanvil that the implementation of this ticket needed a bit more effort and action than the IB stated, mostly due to:

  • There could not simply just be a new cell added to the footer output, this resulted in the CTA and existing footer elements sitting on different horizontal planes
    • I had to introduce dynamic cell sizing based on large, medium and small viewports
    • This poses a risk that if the existing footer elements are widget than the current simple "source" element, it may still push footer and CTA to separate horizontal planes, we may need to consider this in future if we introduce longer footer elements
  • The new badge needed to have a fixed height set, as if not set it would adopt the height of the subtitle/subheader area (due to flex) and it would look more like a circle. Providing a fixed height takes care of this

I will update the IB based on this, cc @benbowler @techanvil

@10upsimon
Copy link
Collaborator

Also noting that due to eslint complexity violations (exceeded by 6 levels of complexity), resulting efforts included the separation of the new badge component into it's own WidgetNewBadge component, accepting a widget area slug as the only parameter. This will be detailed in the IB following updates.

@mohitwp
Copy link
Collaborator

mohitwp commented Aug 19, 2024

QA Update ✅

  • Tested on dev environment.
  • Verified CTA and NEW badge position is now as per Figma and AC in mobile, tablet and Desktop.
  • Tested on mobile, tablet and desktop viewports.
  • Tested on 600px, 601px, 783px, 960px and 961px and above.

Mobile -

image

image

image

Tablet -

image

image

image

image

Desktop -

image

image

@mohitwp mohitwp removed their assignment Aug 19, 2024
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 S Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests