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 292 forks source link

Update key metrics selection to persist the current selection group #9385

Closed zutigrm closed 2 days ago

zutigrm commented 2 months ago

Feature Description

Metric selection logic will need some logic updates. Currently metrics are categorised in available and saved lists, but the new changes to the selection panel will require storing metrics in the current selection group which should include all selected items. Items should be persisted across groups, and show as checked/unchecked opposed to the current implementation where items switch lists based on the saved items.

On high overview, new form lists should be created, so final lists would look like this:

This logic should reflect in ChipTabGroup component which should have all the groups and selection working correctly and style of the selection panel is matching the Figma design

Check Changes to the saved metric items and persisting the current selection of the design doc


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

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

A note for QA to verify issues that were raised under https://github.com/google/site-kit-wp/issues/9378

Changelog entry

aaemnnosttv commented 2 months ago

Also every other metric item that was selected from any other group while the selection panel is open.

@zutigrm The effective selection as defined originally is independent of state changes in the panel so this part of its definition above should be removed.

Staged metrics list

  • Everything that user have selected within the currently active group will be temporarily kept here, once group is switched, or save button clicked, this list will be transferred over to the Effective selection metrics list, and Staged list is reset (emptied).

This might be more clearly defined as "Unstaged" as this list is only necessary to temporarily hold deselected metrics that were toggled on the current metrics view. It is then reset when the selection changes from a different view, or the edit metrics session is closed. It's unnecessary to mention transferring this list since it is only concerned with temporary holding unselected metrics. Only Selected metrics are relevant for saving which are already clearly defined separately. I think this may have been confused by referring to this set before as [any metrics which were selected regardless of the current state] but that's not necessary.

The Selected group would be more accurate to think of as Staged meaning those that will be saved on confirmation. It's probably more clear to understand them as Selected as already mentioned though, as that is hopefully more explicit and should be less open to misinterpretation :)

zutigrm commented 1 month ago

@aaemnnosttv

The effective selection as defined originally is independent of state changes in the panel so this part of its definition above should be removed.

Initially, it will reflect what is on the dashboard - meaning it will be collected from the selector that either retrieves selection from manual input, or tailored metrics that are being shown. But afterwards, this list should include a total list of everything that is going to be used for rendering the Current selection tab. Basically this list will be used for sourcing the metric items under the Current selection group tab. It doesn't have much of an usage/doesn't serve a purpose if it is only static reflecting the initial dashboard selection, since once panel is opened, user will make selections, and those items that were selected, and temporaty stored in Unstaged list will be transferred over to the Effective list - which is rendering all the items that were added under Current selection group, and then determines which metric items should be shown as checked by checking if each metric item slug from Effective list is located in Selected list. That way we keep the list containing all the items that are both checked and unchecked, so they are properly showin in CUrrent selection.

This might be more clearly defined as "Unstaged" as this list is only necessary to temporarily hold deselected metrics that were toggled on the current metrics view.

Good point, updated

It is then reset when the selection changes from a different view, or the edit metrics session is closed. It's unnecessary to mention transferring this list since it is only concerned with temporary holding unselected metrics

Techically this list will hold selected metrics, as the final choice from the currently active group, and then on tab switch, etc, those should be pushed to the Effective list so they would be shown under Current selection per above comment

aaemnnosttv commented 1 month ago

Thanks @zutigrm – apologies I wasn't able to get back to this sooner. It seems there may be some confusion still about some aspects of the behavior here so I've synced with @10upsimon and he'll take over the remaining review and approval.

10upsimon commented 1 month ago

@zutigrm after syncing with @aaemnnosttv and having another sync with you, we are indeed all on the same page with regards to requirements. AC is ✅ and moving this over to IB.

10upsimon commented 1 month ago

@zutigrm I assigned this to you for IB following our recent sync.

zutigrm commented 1 month ago

Returned it from IBR to update IB to sync with some changes made during the execution of 9378

zutigrm commented 4 weeks ago

For the IBR reviewer: Assign this issue to me once it pass IBR, since it is an extension of the issue I worked on and will be easier for me to continue working on it here

10upsimon commented 3 weeks ago

@zutigrm IB LGTM ✅ Moving to EB and assigning to you.

kelvinballoo commented 5 days ago

QA Update ⚠

Most of the items are working fine. I have a couple of issues to flag however:

ITEM A: ⚠ Whenever I am closing the selection panel and then reopening it again, it doesn't start with a clean slate of the current selection.

https://github.com/user-attachments/assets/6e912213-65d6-4ecd-b772-a766f4b21585

ITEM B: ⚠ When I have metrics selected and I turn on the feature flag for Conversion reporting, I can see the message 'no metric was selected yet' when actually there are metrics.

https://github.com/user-attachments/assets/e9be1565-28d3-40df-99a0-0e300e2c2a93

ITEM C: ⚠ When feature flag is off, the error message flags max selection as 4. Is that as expected? Or should we move it to 8?

![Image](https://github.com/user-attachments/assets/218c9f7b-71fb-4b81-96c5-4d2f06885a71)

ITEM D:

This can be tackled as a separate ticket. When I check on iPad 10 safari, I see some inconsistencies. Documented below.

For the 'New visitors' tile, there is some overlapping text. Also for the other tiles, I feel the content is very close to the title of each tile. ![Image](https://github.com/user-attachments/assets/3756cf7b-f111-4356-a154-b65ac4d69378) Notice how the 3rd tile is bigger than the first 2, when it's the same information being displayed. ![Image](https://github.com/user-attachments/assets/17afe826-f878-414e-8946-510cb80a2a20)

ITEM E: ⚠ Referring to the last part of the QAB: Groups that have no metrics - Generating leads and selling products, will only show when events are detected. On GA4 property with no conversion events, these two groups will not be visible


Other than that, most things have been verified good:

Also, items 4-11 from https://github.com/google/site-kit-wp/issues/9378 were tested good as well: ITEM 4:Checkbox is 18 x 18 ✅

![Image](https://github.com/user-attachments/assets/578cb7e6-ba29-48b4-8daa-e0e0fe351980)

ITEM 5: Checkboxes are vertically aligned ✅

![Image](https://github.com/user-attachments/assets/77b1c64c-be5a-46f8-a9ec-644656131a75)

ITEM 6 : Covered above in main issue. ✅

ITEM 7: Font is 14 px ✅

![Image](https://github.com/user-attachments/assets/5b31122a-c2e3-42a0-bf1d-e140a6d427fa)

ITEM 8: Font is 12px on mobile. ✅

![Image](https://github.com/user-attachments/assets/609445c5-e7a2-49d3-bd5e-28a21c07972c)

ITEM 9: Alignment is as expected. ✅

![Image](https://github.com/user-attachments/assets/366d3082-c95f-4dc6-959d-0faf96dd5a58)

ITEM 10: The message sticks to the bottom of the panel and doesn't move from group to group. ✅

https://github.com/user-attachments/assets/39cfa512-e137-4d99-84bd-581b91c88a8b

ITEM 11: Settings font weight is 400. ✅

![Image](https://github.com/user-attachments/assets/28b75939-2882-4051-8b26-d3f41f671441)
zutigrm commented 5 days ago

Thanks @kelvinballoo

ITEM C: ⚠ When feature flag is off, the error message flags max selection as 4. Is that as expected? Or should we move it to 8?

This is correct, max selection of 8 items is part of conversion reporting feature, max selection of 4 items is the existing limit until feature is released

ITEM D: ⚠

This can be tackled as a separate ticket. When I check on iPad 10 safari, I see some inconsistencies. Documented below.

Can you please open a new issue for this and ping me with it, I will include it in the epic and get it moving through definition

ITEM E: ⚠ Referring to the last part of the QAB: Groups that have no metrics - Generating leads and selling products, will only show when events are detected. On GA4 property with no conversion events, these two groups will not be visible

Is there an easy way to test the conversion items? I don't have any site for that. Or it's not required at this point and will be tested under another ticket?

Yes, there is a test website, please ping me on slack so I can add you and send you the link

Rest of the items A & B are fixed in a follow up

kelvinballoo commented 3 days ago

QA Update ⚠


Other than that, most things have been verified good:


Also, items 4-11 from https://github.com/google/site-kit-wp/issues/9378 were tested good as well: ITEM 4:Checkbox is 18 x 18 ✅

![Image](https://github.com/user-attachments/assets/578cb7e6-ba29-48b4-8daa-e0e0fe351980)

ITEM 5: Checkboxes are vertically aligned ✅

![Image](https://github.com/user-attachments/assets/77b1c64c-be5a-46f8-a9ec-644656131a75)

ITEM 6 : Covered above in main issue. ✅

ITEM 7: Font is 14 px ✅

![Image](https://github.com/user-attachments/assets/5b31122a-c2e3-42a0-bf1d-e140a6d427fa)

ITEM 8: Font is 12px on mobile. ✅

![Image](https://github.com/user-attachments/assets/609445c5-e7a2-49d3-bd5e-28a21c07972c)

ITEM 9: Alignment is as expected. ✅

![Image](https://github.com/user-attachments/assets/366d3082-c95f-4dc6-959d-0faf96dd5a58)

ITEM 10: The message sticks to the bottom of the panel and doesn't move from group to group. ✅

https://github.com/user-attachments/assets/39cfa512-e137-4d99-84bd-581b91c88a8b

ITEM 11: Settings font weight is 400. ✅

![Image](https://github.com/user-attachments/assets/28b75939-2882-4051-8b26-d3f41f671441)
kelvinballoo commented 3 days ago

QA Update ✅

Moving ticket to approval and everything is working as expected.


Other than that, most things have been verified good:


Also, items 4-11 from https://github.com/google/site-kit-wp/issues/9378 were tested good as well: ITEM 4:Checkbox is 18 x 18 ✅

![Image](https://github.com/user-attachments/assets/578cb7e6-ba29-48b4-8daa-e0e0fe351980)

ITEM 5: Checkboxes are vertically aligned ✅

![Image](https://github.com/user-attachments/assets/77b1c64c-be5a-46f8-a9ec-644656131a75)

ITEM 6 : Covered above in main issue. ✅

ITEM 7: Font is 14 px ✅

![Image](https://github.com/user-attachments/assets/5b31122a-c2e3-42a0-bf1d-e140a6d427fa)

ITEM 8: Font is 12px on mobile. ✅

![Image](https://github.com/user-attachments/assets/609445c5-e7a2-49d3-bd5e-28a21c07972c)

ITEM 9: Alignment is as expected. ✅

![Image](https://github.com/user-attachments/assets/366d3082-c95f-4dc6-959d-0faf96dd5a58)

ITEM 10: The message sticks to the bottom of the panel and doesn't move from group to group. ✅

https://github.com/user-attachments/assets/39cfa512-e137-4d99-84bd-581b91c88a8b

ITEM 11: Settings font weight is 400. ✅

![Image](https://github.com/user-attachments/assets/28b75939-2882-4051-8b26-d3f41f671441)