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

Notices and NoticeNotifications should follow mobile styles in tablet viewport

Open kelvinballoo opened this issue 1 year ago • 5 comments

Feature Description

Noticed this during RRM but we can review this across the board for subtle notifications. On tablet, the banners just look weird in my opinion. Refer to the picture attached.

  • There is a very dense block of text and the buttons sections have a lot of space.
  • This feels unbalanced.
  • I'm wondering if it might be best to move the buttons below and let the text sit along the width of the banner.
Screenshot 2024-08-13 at 18 04 07

UPDATE: Changing the layout of notices simply based on the size of viewport doesn't work for us when certain notices are rendered in narrow components, e.g. in #11459 (Figma). So we should determine the layout of a notice based on the width of the notice itself and not the viewport width.


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

Acceptance criteria

  • All Site Kit "Notices" (i.e. <Notice> and <NoticeNotification> components) should be styled as per their corresponding Figma mocks based on the width of the notices themselves.
  • If the width of a notice is <= 800px, use the Mobile and Tablet designs.
  • If the width of a notice is > 800px, use the Narrow Desktop and Wider Desktop designs.

Implementation Brief

** The following IB is implemented in this https://github.com/google/site-kit-wp/pull/11954 PR as a PoC. Tests will need to be fixed. **

  • Within assets/js/components/Notice/index.js:
    • [x] Wrap the entire component being returned with another div with a className of googlesitekit-notice-container.
    • [x] Move the ref to this parent container now.
  • Within assets/sass/components/notice/_googlesitekit-notice.scss:
    • [x] For the googlesitekit-notice-container selector, add a container-type: inline-size property.
    • [x] Change all media queries for bp-tablet to be @container (width > 800px)
    • [x] Remove unnecessary 10px bottom margin for the p tag within .googlesitekit-notice__content which was incorrectly added previously.
  • [ ] Update all VRTs and fix JSUnit snapshots.

Test Coverage

  • No new tests required.

QA Brief

Changelog entry

kelvinballoo avatar Aug 20 '24 17:08 kelvinballoo

@sigal-teller

For some reason, we designed all "Notice" style notifications to follow the desktop layout even for tablet viewports where the buttons are next to the text.

As shown in the Feature description screenshot, this looks ugly when there is a long title, description and multiple long CTAs in the case of the RRM notice. And it would look better when it is fixed as per the Figma mock (the fixed version is below): Image

However, this does mean that several other notices, which do not have long titles, descriptions or many CTAs, do look a bit empty and funny in this layout as shown below: Image

So what would be your guidance here? Should we keep things as they are? Or should we update all notices at tablet viewport to follow the "mobile/table" multi-row design as per the Figma mocks?

I wanted to confirm this before we proceed here as even though the code change is small, it will be a considerable effort to test all affected notices in the plugin!

jimmymadon avatar Nov 19 '25 10:11 jimmymadon

Hey @jimmymadon For the tablet view we definitely need to follow the Figma design which places the CTA buttons below the title and text, and not to the right. Longer texts create an unbalanced view which doesn't look good (like the screenshot in the description in this issue). As for the screenshots you've added, I'm not sure I understand what you meant. It looks like the width specified is 959px, which is above the breakpoint for tablet, which mean it should use the "Narrow desktop viewport" (buttons to the right of the text and not below). Perhaps I'm missing something here?

sigal-teller avatar Nov 19 '25 17:11 sigal-teller

@sigal-teller My apologies, the screenshots I've shared are for Site Kit's tablet view breakpoint which ends at 960px and not 900px (as mentioned in the Figma mocks). We had discussed this and could just update the notes in Figma to ensure we don't have this confusion in the future. We don't seem to have a breakpoint between 600px and 960px. So the screenshots are at the highest possible tablet viewport which shows the extra whitespace when we use the tablet layout.

jimmymadon avatar Nov 19 '25 17:11 jimmymadon

Thanks @jimmymadon for clarifying. In this case I will update the notes in Figma to match the actual breakpoints. As for this issue, we've created a system, which includes several notices like success, errors, warnings etc. It's true that success notices are potentially shorter then error or warning which includes longer texts, but we need to take into account those long texts as well. I did try both options at the time as we know that some notice variants won't look as good as planned. I find having some blank space in corner cases is better then creating a unbalanced view with narrow paragraph that breaks into too many lines.

sigal-teller avatar Nov 19 '25 17:11 sigal-teller

AC + IB ✔️

eugene-manuilov avatar Dec 03 '25 17:12 eugene-manuilov

I'm changing the priority of this as it was highlighted by multiple testers in the PUE v0 bug bash so it is being noticed actively specifically in this epic so it should be addressed alongside the bug bash issues for this epic.

benbowler avatar Dec 19 '25 10:12 benbowler

AC and IB ✔️

eugene-manuilov avatar Dec 19 '25 11:12 eugene-manuilov