google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 286 forks source link

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

Closed kelvinballoo closed 1 month ago

kelvinballoo commented 3 months ago

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


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

Acceptance criteria

Widget area CTA position

Widget area "New" badge position

Implementation Brief

Test Coverage

QA Brief

Changelog entry

nfmohit commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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.

nfmohit commented 3 months ago

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 commented 3 months ago

IB ✅ Thanks @benbowler !

10upsimon commented 1 month ago

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:

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

10upsimon commented 1 month ago

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 commented 1 month ago

QA Update ✅

**Mobile -** ![image](https://github.com/user-attachments/assets/1b82a955-74ab-4ce7-ad6d-277413bf7ae5) ![image](https://github.com/user-attachments/assets/c138b71b-d513-493a-9c79-af2e8cc8aeee) ![image](https://github.com/user-attachments/assets/ae5d8701-b8d5-4305-9f77-5b2b504cdf23) **Tablet -** ![image](https://github.com/user-attachments/assets/1a681c3c-78de-4917-a358-aa0da19bf232) ![image](https://github.com/user-attachments/assets/6596c211-0548-45ed-99d9-217a57a44ba9) ![image](https://github.com/user-attachments/assets/fd6b0aa0-c180-49a4-b8c6-b3d1b6c612a8) ![image](https://github.com/user-attachments/assets/599f58b7-bc27-4951-b632-91e893ef0c2e) **Desktop -** ![image](https://github.com/user-attachments/assets/7a4c2448-6572-469c-abbb-ea4f5f16fed4) ![image](https://github.com/user-attachments/assets/2e7b27a0-05d0-421f-9f3f-669046c16447)