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

Glitches / errors when changing the audience selection in mobile viewports #9168

Closed kelvinballoo closed 1 month ago

kelvinballoo commented 2 months ago

Bug Description

A number of glitches have come to light in the Audience Tiles view. These should be looked at together as it's likely the fixes will be related.

Steps to reproduce

Audience ordering goes out of sync

  1. Given a property is connected where the SK audiences are in the order "new visitors", "returning visitors".
  2. Load the dashboard with the "new visitors" audience selected.
  3. Open the Selection Panel, add the "returning visitors" audience, and save the selection.
  4. The audiences will be in the order "new visitors", "returning visitors" in the Audience Tiles widget, but "returning visitors", "new visitors" in the Selection Panel.
Screen capture https://github.com/user-attachments/assets/ddd75505-8e00-4b67-b50b-f53807f17fbe

Alternatively:

  1. Load the dashboard with the "returning visitors" audience selected.
  2. Open the Selection Panel, add the "new visitors" audience, and save the selection.
  3. The audiences will be in the order "returning visitors", "new visitors" in both Audience Tiles widget" in the Selection Panel, which is opposite to their order in the API.

Adding audience before the last tab breaks the new audience tab and the last tab

  1. Load the dashboard in a mobile viewport with two audiences selected.
  2. Open the Selection Panel, add another audience which appears before the last audience in the tab order, and save the selection.
  3. The tabs for the newly added audience and the last audience will glitch and open the wrong audiences.
Screen capture https://github.com/user-attachments/assets/369479fa-4102-422b-9b04-832fe7ff584d

Removing audience does not select the remaining tab

  1. Load the dashboard in a mobile viewport with two audiences selected.
  2. Open the Selection Panel, remove the second audience, and save the selection.
  3. The tab for the remaining audience will not be selected and its tile will not be visible.
Screen capture https://github.com/user-attachments/assets/fbda7830-fa25-44db-9102-a5407b34511b

Removing the first or second audience with three selected breaks the remaining tabs

  1. Load the dashboard in a mobile viewport with three audiences selected.
  2. Open the Selection Panel, remove the first or second audience, and save the selection.
  3. The tabs for the remaining audiences will glitch and not select properly.
Screen capture https://github.com/user-attachments/assets/aad12d4e-a99b-4195-b44d-5f5e4efa7cf8

Removing and readding an audience breaks the audience tab. Adding a third audience results in a JS error when clicking the tab.

  1. Load the dashboard in a mobile viewport with two audiences selected.
  2. Open the Selection Panel, remove second audience, and save the selection.
  3. Open the Selection Panel, re-add the audience, and save the selection.
  4. The tab for the audience will glitch and not select properly.
  5. Open the Selection Panel, add a third audience, and save the selection.
  6. Clicking on the tab for the new audience will result in a JS error captured and displayed in the Audience Tiles widget.
Screen capture https://github.com/user-attachments/assets/771d8a4f-e21a-4158-b5f0-abcdf110c93f

Notes

Additional Context


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

Acceptance criteria

Implementation Brief

Audience ordering

Remember the previously active tab when the audience selection changes

TabBar glitches

The glitchy behaviour outside of the audience ordering is due to the TabBar implementation not handling updates to the tabs very well. This is because it maintains an internal list of tabs which it pushes to when it sees a new child element (see ref: this.pushToTabList in renderTab), but doesn't reorder/update when the children are reordered or removed.

We need to apply the following workarounds in the AudienceTiles component:

Draft PR

There is a draft PR that can be completed: https://github.com/google/site-kit-wp/pull/9228

Test Coverage

QA Brief

Changelog entry

kuasha420 commented 1 month ago

Thanks for the investigation and the IB here @techanvil. All LGTM.

IB :heavy_check_mark:

kelvinballoo commented 1 month ago

QA Update ✅

Verified good on real iPhone 15 Pro Max Safari. Ticket is good to move to approval.