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

Improve the layout of the graphic on the Consent Mode Setup CTA Banner at narrow desktop viewports.

Open techanvil opened this issue 1 year ago • 16 comments

Feature Description

The layout of the graphic on the Consent Mode Setup CTA Banner at narrow desktop viewports could be improved. As can be seen here at 961px the white and grey window in the middle of the graphic is slightly clipped:

image

It would be good to avoid this clipping and retain some of the green background around it as can be seen here at a wider viewport:

image


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

Acceptance criteria

  • The Consent Mode Setup CTA Banner's layout should be vertically stacked, with the text and CTAs on one row and the graphic displayed on a row beneath it, for the following viewports, as per the linked Figma designs:
    • Mobile (viewport width <= 450px): Figma
    • Tablet (viewport width <= 600px): Figma
    • "Narrow" desktop (viewport width <= 1280px): Figma
  • For the "wide" desktop viewport (width >= 1281px), the layout should flow horizontally, with text/CTAs and the graphic on a single row, with the graphic to the right, as per the Figma design.

Please refer to the actual Figma designs as the source of truth, but for the sake of illustration here are screenshots of the layouts from Figma:

Mobile: image

Tablet: image

Narrow desktop: image

Wide desktop: image

Implementation Brief

  • [ ] Update assets/js/components/consent-mode/ConsentModeSetupCTAWidget.js:
    • [ ] Refactor the component to use divs with classes rather than Grid, Row and Cell components, because these components don't support the wide desktop breakpoint we need.
  • [ ] Update the layout of the CTA based on the AC in assets/sass/components/ads/_googlesitekit-ads-setup-banner.scss, use a mobile first approach and update the layout using flexbox, switching the flex direction to position the svg beside or below the widget text depending on the breakpoint.

Test Coverage

  • Update VRTs based on the new UI changes.

QA Brief

Changelog entry

techanvil avatar Mar 11 '24 18:03 techanvil

I'll create a design for narrow/tablet viewport

sigal-teller avatar Jun 13 '24 09:06 sigal-teller

@techanvil I added the tablet/narrow viewport design for the banner. LMK if there's anything else required.

sigal-teller avatar Jun 13 '24 15:06 sigal-teller

Thanks @sigal-teller, that's great for the tablet view. However, what's still not entirely clear is how the graphic should display at the narrower desktop viewports.

Currently the desktop layout has the graphic on the right hand side but it crops at narrower desktop widths as seen in the images above. I wouldn't imagine we want to have the graphic displaying at the bottom for some of the desktop viewports, would we?

What would be great is if you could provide a design at the narrow desktop viewport of 901px to help clarify this further...

techanvil avatar Jun 18 '24 11:06 techanvil

@techanvil Well actually in the narrow desktop viewport I would use the tablet design. There's too much content in this banner to fit in the same layout as wider viewports. The texts breaks into too many lines and the banner looks unbalanced. I think that in this case in narrow viewports we should adapt the design to look batter rather then match the wider viewports design. I added a design for it here.

sigal-teller avatar Jun 18 '24 13:06 sigal-teller

Thanks @sigal-teller. In that case, would you like to specify the breakpoint at which we transition from the tablet/narrow desktop layout to the full width desktop layout? Or, we could simply specify that we should break at the point where the graphic isn't horizontally cropped in the full width layout (maybe also specifying some minimum padding at the horizontal edges too). WDYT?

techanvil avatar Jun 18 '24 13:06 techanvil

@sigal-teller please could you respond to the above q? Just aware you will be on PTO next week so would like to keep this small one moving if possible. Thanks!

binnieshah avatar Jun 20 '24 09:06 binnieshah

@techanvil If that's easier to implement then yes, we can define that we're using the wide desktop viewport as long as the graphics isn't cropped. If it's easier to set a fixed breakpoint that's we can do that as well, we might be able to use one of our existing breakpoints, depending if they are close enough.

sigal-teller avatar Jun 20 '24 09:06 sigal-teller

Hey @sigal-teller, thanks for your reply.

I've been looking into this one a bit more, and, actually I am thinking we should reconsider the approach here. Sorry I didn't think of this earlier when asking my question about the breakpoints.

We can take the approach of breaking at a different breakpoint - but, it would be preferable not to. Bearing in mind we already have multiple banners with a similar design, as seen below (and no doubt more to come), it would be good to use a single generic component for all of these - this is a refactor we can make in the forthcoming Banner Notifications epic.

Ads: image

Consent Mode: image

RRM: image

For the sake of UI consistency and code simplicity, it would be preferable if these all had the same responsive behaviour when it comes to their breakpoints.

What if we take another look at how the graphic is used within the breakpoint? I realise it arguably looks a bit off with it being cropped in the middle of the banner. However, we're already cropped at the top and bottom in desktop viewports. What if we adjusted the size and position of the graphic to avoid cropping on the left, but allow the right to be cropped, where it looks more natural as it aligns with the end of the banner?

Admittedly it's not perfect, but here's how it could look at 961px. As a compromise, for the sake of consistency and using standard breakpoints I think it could be OK.

image

Alternatively - personally, I don't actually think the left hand crop is so bad as long as the foreground graphic elements are not cropped and are well positioned within the banner. We could specify that the main "window" graphic should be centered within the right hand of the pane and only the green background can be cropped within this area, for example:

image

We can of course still take the approach of a different breakpoint if needs be - I don't want to suggest overly compromising the aesthetic integrity. But, as discussed above it would be nice to avoid it if we reasonably can.

What do you think?

techanvil avatar Jun 20 '24 12:06 techanvil

Hey @techanvil I think you're right and we should strive for consistency in all setup banners, since they so use the same layout. i think there are 2 main issues here:

the 1st issue is to avoid cropping the visual, unless it's on the right edge of the banner. I think that this can be solved by creating a shape that can be displayed uncut when it's scaled down to fit the remaining space. I can do that for each green shape that I'm using, and then it will only get cut in wider viewports that will allow more space for the text.

The 2nd issue is switching to tablet layout in narrow viewports. The reason why I preferred this design in narrow viewports is to avoid an unbalanced layout, where the text is stacking on the left while increasing banner height significantly, and as a result we have a small illustration on the right side of the banner. It looks unbalanced. I think that maybe we should use this design for all banners in narrow viewports (I assume we can do that while refactoring all banner).

WDYT?

sigal-teller avatar Jul 01 '24 10:07 sigal-teller

Thanks @sigal-teller. Adjusting the graphics as you've described SGTM.

With regard to 2nd issue - if we decide to use the vertically stacked layout for narrow desktop viewports as per your design, it does raise the question of what viewport width this layout should be applied up to. Our standard desktop breakpoints are 961px and 1281px. Here's a mockup of how it could look at the top end of this range, 1280px. It's OK, but the horizontal layout is probably better. Do you think this would be an acceptable layout at this width (presumably with a tweak to the graphics as per your updated design)?

image

It's also worth considering that if we are going to make changes to all of these banners, it might be better to undertake the work as part of the Banner Notification Refactoring epic that @jimmymadon has picked up the lead on... Cc @binnieshah

techanvil avatar Jul 01 '24 17:07 techanvil

Thanks @techanvil this may make sense if we are going to make changes to the banners. @sigal-teller let us know what you think on what Tom has said above. I can then revise the sprint plan accordingly. Thanks

binnieshah avatar Jul 02 '24 07:07 binnieshah

@techanvil I don't think it will be a good idea to use this design for all desktop viewports, including the wider viewports. Stacking the text above the illustration creates unbalanced layout and the banner height gets extended more then it needs.

We can use the breakpoints you mentioned, so from 1281px (net 1121 for the SK view without WP nav bar) we can use the wider desktop viewport design as it is here, meaning the text will be placed to the left of the illustration.

Tablet and narrow desktop viewports will have the vertical stacking like here.

Does that make sense?

sigal-teller avatar Jul 02 '24 18:07 sigal-teller

@techanvil @binnieshah Just a quick note that as part of the first 2 phases of the Banner Notifications Refactoring epic, we don't intend to work on "inline" banners, i.e. banners which appear within the dashboard widget areas rather than within the Header component. We also don't intend to specifically consolidate the design or make any design changes unless it specifically makes the refactored code much simpler in a case here and there. But we will get to these eventually once the actual "notifications" are all refactored.

c.c. @aaemnnosttv

jimmymadon avatar Jul 02 '24 23:07 jimmymadon

OK, so sounds like it would be good to get this one done within an upcoming sprint. @techanvil please could you review Sigal's note above and confirm this is OK? I can then assign it within the team. Thanks

binnieshah avatar Jul 03 '24 08:07 binnieshah

Thanks @binnieshah.

@sigal-teller, that makes sense, and it's great you have created the designs for the different viewports for the Consent Mode banner.

I think, for the sake of moving this issue along we can address the CoMo banner in isolation, and I've added AC.

We can then follow up with a separate issue to apply the same responsive layout to the Ads and RRM banners. It would be great to flesh these out with per-viewport designs as well, to get a visual check on how they would look prior to committing to implementation, and in case the graphics might need a tweak. @sigal-teller, would you be able to provide designs for these?

techanvil avatar Jul 03 '24 11:07 techanvil

IB ✅

tofumatt avatar Jul 04 '24 14:07 tofumatt

QA Update ⚠️

@zutigrm Layout for the tablet is not matching with Figma design. As per AC Tablet (viewport width <= 600px): Figma Can you please check if AC is correct ?

Tablet viewport

image

Wide Dashboard

image

Narrow Dashboard

image

Mobile Dashboard

image

mohitwp avatar Jul 16 '24 05:07 mohitwp

Thanks for raising it @mohitwp . It is most likely written by mistake/typo during AC, as we usually consider tablets starting from 600px not bellow, it is already mobile bellow that width. Let me confirm this with @techanvil since he wrote the AC

zutigrm avatar Jul 16 '24 07:07 zutigrm

Thanks @mohitwp and @zutigrm - this was a mistake in the AC as Aleksej has pointed out. I have now corrected it, sorry for the inconvenience!

techanvil avatar Jul 16 '24 08:07 techanvil

QA Update ✅

  • Tested on dev environment.
  • Verified Consent Mode setup CTA banner appearing at per Figma files.

Tablet view

image

image

Mobile View

image

Wide Dashboard

image

Narrow Dashboard

image

mohitwp avatar Jul 16 '24 11:07 mohitwp