search

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.25k stars 291 forks source link

Top content widget: Pageviews and Unique Pageviews table headers overlap at a 601px wide viewport. #5561

Closed techanvil closed 5 months ago

techanvil commented 2 years ago

Bug Description

On the "Top content..." widget, the Pageviews and Unique Pageviews table headers overlap at a 601px wide viewport.

Steps to reproduce

  1. View the Dashboard with a 601px viewport.
  2. Scroll to the "Top content..." widget
  3. See the overlapping table headers.

Screenshots

Version 1.79.0 RC1: image.png

Notice how the overlap has increased slightly compared to the pre-GM2+ styling.

Version 1.78.0: image.png

Additional Context

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

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

aaemnnosttv commented 2 years ago

@techanvil the change here looks rather small but looks like there is definitely room for improvement at this viewport in general. Any ideas how we could better adapt this to smaller viewports?

techanvil commented 2 years ago

Hey @aaemnnosttv, having dug into this a bit, I think the most appropriate fix would be to slightly reduce the width of the Title column (currently 50%) to allow more space for the metric columns. It would need to be reduced to about 44% and this could be applied only at the tablet viewport of 601-960px (or across all viewports for simplicity/consistency's sake).

Title column width 44%, viewport width 601px: image

Alternative solutions

CSS overflow and ellipses

We could apply CSS overflow and ellipses rules, and add a tooltip to the headers. The obvious disadvantage being the text is no longer fully visible and the user would need to interact with the tooltip to read it.

image

Reduce the font.

We could arguably reduce the font, but this also seems a bit of an excessive fix.

Font reduced to 12px: image

Hyphenation

I did spend some time looking into hyphenation of the word "pageviews" as an option, and had initially thought to suggest this as the proposed fix, but having slept on it I have realised that it's not such a great idea as it would mean compromising the current view by hyphenating "pageviews" over a relatively wide range of viewports (from 601px up to 827px in my testing) for the sake of fixing a mild overlap problem that occurs at a narrower range of viewports (the existing overlap occurs between 601px and ~660px).

I'll leave my findings on hyphenation here for the sake of completion:

The approach I was considering is to allow the word "pageviews" to be hyphenated to avoid the overflow.

I can see that we have applied CSS hyphenation to the table headers as a general fix to avoid overflowing - as seen here in commit https://github.com/google/site-kit-wp/pull/749/commits/ac63a441cafa554fea3663f5646db02521e53e12.

Looking at the table in other languages it can be seen in action, for example here in German:

image

Note the overflow having unticked the hyphen: auto rule in devtools:

image

The problem with the English version is that "pageviews" is not a standard word and the browser doesn't know how to hyphenate it.

The approach I was therefore considering is to let the browser know that "pageviews" can be hyphenated by inserting a soft hyphen into the word, i.e. 'Page\u00ADviews'. We could create a utility function to keep the code clean and reusable rather than inlining this funky string. It works as expected - and should gracefully degrade where not supported.

At 601px: image

When the screen is wide enough (827px in this case): image

Interested to hear your thoughts!

aaemnnosttv commented 2 years ago

Thanks @techanvil! I had no idea soft hyphens were a thing!

I feel like the problem is rooted in showing more than 2 columns in a relatively small space. That is, if it were just Title and one more metric, it would be fine but we're cramming in a few and the column headings are hard to fit well, especially when you start considering i18n.

I'd be interested to see how feasible it would be to use icons to represent the headings (other than Title). E.g.

These are just ideas from the icon set but then we could also keep the current names as tooltips for accessibility and clarity as to what it means.

aaemnnosttv commented 1 year ago

@sebastianmantel any suggestions for what we could do here?

sebastianmantel commented 1 year ago

@aaemnnosttv we can change the font style of those parameters to 12px [body-small] and that will fix the issue. WDYT?

techanvil commented 5 months ago

Closing as this is covered by the more recent #7563, although the findings of this issue will still be relevant.