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

Issues with Most Popular Pages module responsiveness on 599px to 668px viewports

Open mohitwp opened this issue 2 years ago • 11 comments

Bug Description

Analytics most popular pages widget is not properly appearing on viewport having width between 599px to 668px.

Steps to reproduce

  1. Set up site kit with Analytics.
  2. Go to main dashboard.
  3. Set viewport between 599px to 668px using dev tool.
  4. See issue.

Screenshots

Screenshots

image

image

image


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

Acceptance criteria

  • For the Top content over 28 days table, on mobile viewports where the table headings start to overlap with each other, the table should be converted to a two column layout with tabs that as per this Figma mock.
Screenshot Screenshot 2024-07-01 at 15 42 04

Implementation Brief

Test Coverage

QA Brief

Changelog entry

mohitwp avatar Sep 11 '23 06:09 mohitwp

@sigal-teller @aaemnnosttv This seems to be an issue only for a very short viewport range between 600px and 670px. I suggest simply wrapping the words, ideally with CSS hyphens. Does this look ok?

Screenshot 2024-05-31 at 02 21 12

jimmymadon avatar May 31 '24 01:05 jimmymadon

Hey @jimmymadon, it's worth noting we have a similar issue in the backlog, https://github.com/google/site-kit-wp/issues/5561, where we looked into hyphenation, among other things - see https://github.com/google/site-kit-wp/issues/5561#issuecomment-1191770816.

However as discussed there are issues with this approach; the direction from Seb toward the end of the issue was to simply change the font size.

techanvil avatar May 31 '24 11:05 techanvil

@sigal-teller Could you please review the solutions proposed by @techanvil and @aaemnnosttv from this comment thread.

For me personally, the hyphens work nicely. But as Tom and Evan say, using icons are also a good option which will keep the height consistent too.

jimmymadon avatar May 31 '24 12:05 jimmymadon

@sigal-teller You suggested adding horizontal scroll which we have introduced for the Audience Segmentation widget. In experimenting with the horizontal scroll, I realised we could further reduce the width of column 1 with minimal code change and effort.

Screenshot 2024-06-19 at 11 50 19

jimmymadon avatar Jun 19 '24 11:06 jimmymadon

Thank you @jimmymadon for these explorations. I think it doesn't look so great especially with long URLs and page names. Does using horizontal scrolling here adds a lot of complexity?

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

@sigal-teller Thank you - horizontal scrolling shouldn't be a big issue. I'll add that as the AC here, move this along and we can see the estimate that comes out from the IB.

jimmymadon avatar Jun 20 '24 13:06 jimmymadon

Having the remaining columns be horizontally scrollable while the first column remains fixed in place is likely significantly more complicated/effort than simply scrolling the entire table, which is probably an easier solution. 🤔

I can kind of understand keeping the first column around for the context, but it will also make it so the user needs to horizontally scroll when using touch gestures ONLY after the first column. That's not intuitive or normal scroll behaviour, especially when there's no design here for a visual indicator that there's more content.

I think we should be scrolling the entire table/widget contents, not select columns. It presents significant accessibility/usability issues, especially on touchscreen devices, where these viewports are likely to be encountered.

tofumatt avatar Jun 25 '24 17:06 tofumatt

FWIW, these originally were designed as horizontally scrollable in smaller viewports and this was later changed for some reason I can't recall.

On a related note, we should scroll the top navigation if it overflows (a separate issue) as this is a common pattern. image

aaemnnosttv avatar Jun 28 '24 18:06 aaemnnosttv

@tofumatt I agree that it's more complicated to scroll only some of the columns in several aspects. The question is if it makes sense to see the numbers in the table without the page names, and if the user will need to scroll back and forth again and again.

@jimmymadon I have another idea here that from a UX perspective is much better but might require more effort. Instead of having 1 table with scrolling we can have 4 tables that will include the 1st column and one more column, and we'll add tabs above the table to switch between them: "Pageviews", "Sessions" etc. This will be vwery similar to the pattern we introduced in the Audience segmentation UX for multiple groups in narrow viewports. I also found this pattern in SC (attaching a screenshot) IMG_9912

@aaemnnosttv As for the top navigation, we should definitely add a horizontal scroll instead of breaking into 2 lines. I suggested it a while ago when I worked on the header improvements which were paused, but I'm using it in all my designs since then. you can see it here for example.

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

@sigal-teller

This will be vwery similar to the pattern we introduced in the Audience segmentation UX for multiple groups in narrow viewports.

If @tofumatt or another AC reviewer (@eugene-manuilov?) can review this solution, it might be better to have a formal design of this in Figma which we can use for all multiple column tables with a wide first column.

jimmymadon avatar Jun 30 '24 23:06 jimmymadon

@jimmymadon I added an example here for the Figma design I referred to in my previous comment. LMK if that can work.

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

That tabbed solution seems great, I think that's a more intuitive one than scrolling; it'd be more straightforward to implement and I think is flexible enough to even allow for more tabs than fit in the widget, by scrolling only the tabs.

Looks good to me, moving to IB 👍🏻

tofumatt avatar Jul 15 '24 21:07 tofumatt

@benbowler, instead of making ReportTable breakpoint aware, we should read breakpoint data in a component that requires an alternative layout for the table based on the current breakpoint. In other words, we should update the report table to have an alternative layout and the component that renders that table to read the current breakpoint and to use an alternative layout if the breakpoint is tablet or mobile.

eugene-manuilov avatar Sep 22 '24 14:09 eugene-manuilov

@eugene-manuilov sounds good, I've updated the IB.

benbowler avatar Sep 23 '24 11:09 benbowler

Thanks, @benbowler. IB ✔️

eugene-manuilov avatar Sep 24 '24 13:09 eugene-manuilov

QA Update: ⚠

@benbowler @techanvil I am confused by the solution we have gone with on this one. I was expecting to see my test site look like the figma designs with the tabbed view. I am on the develop branch and using data from oi.ie. While this is a new site, I did clear cache and refresh the develop branch a few times.

This is what I see at 599 px Image

This is what I see at 600 px Image

The overlapping continues throughout to 668 px.

Am I missing something here?

wpdarren avatar Dec 05 '24 08:12 wpdarren

Thanks for flagging this @wpdarren, it turns out the PR was approved but not merged although the rest of the process was completed with the issue moved to QA. This was clearly an oversight so I've gone ahead and merged the PR.

techanvil avatar Dec 05 '24 09:12 techanvil

QA Update: ✅

Verified:

Love the solution for this on mobile! Looks great.

  • For the Top content over 28 days table, on mobile viewports where the table headings start to overlap with each other, the table is converted to a two column layout with tabs that as per this Figma mock.
  • I checked the page title font is also bold when viewed in the desktop viewport, and in the WP dashboard "top content" widget as discussed in Slack.

https://github.com/user-attachments/assets/edffe7a6-db34-4c1c-8aef-8a3fbdff790a

Screenshots

Image Image

wpdarren avatar Dec 05 '24 09:12 wpdarren