Open mohitwp opened 1 year ago
@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?
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.
@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.
@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.
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 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.
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.
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.
@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)
@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
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 I added an example here for the Figma design I referred to in my previous comment. LMK if that can work.
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 👍🏻
@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 sounds good, I've updated the IB.
Thanks, @benbowler. IB ✔️
Bug Description
Analytics most popular pages widget is not properly appearing on viewport having width between 599px to 668px.
Steps to reproduce
Screenshots
Screenshots
![image](https://github.com/google/site-kit-wp/assets/94359491/747eea53-1e4d-4324-bf82-ee7ae1de8e9c) ![image](https://github.com/google/site-kit-wp/assets/94359491/db922390-039a-4862-94ef-8e5c51c920e9) ![image](https://github.com/google/site-kit-wp/assets/94359491/05e824a6-f0bc-44a7-848a-2ca15cf22229)Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Screenshot
Implementation Brief
Report Table Tabbed View
assets/js/components/ReportTable.js
tabbedLayout
, if this prop is true, render the following alternate component structure:TabBar
andTab
components to render a new tab bar above the report table, mapping over thecolumns
props to get the tab titles and keys.assets/sass/components/global/_googlesitekit-table.scss
, matching the Figma mock for this tabbed version of the report table.Most Popular Pages module
assets/js/modules/analytics-4/components/module/ModulePopularPagesWidgetGA4/index.js
:useBreakpoint
hook to get the current breakpoint.tabbedLayout
prop of the ReportTable in this component if the breakpoint isBREAKPOINT_TABLET
orBREAKPOINT_MOBILE
.Test Coverage
assets/js/components/ReportTable.stories.js
, create a new VRT for the tabbed version of this component.QA Brief
Changelog entry