site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard
verified publisher icon google

Update key metrics selection to persist the current selection group

Open zutigrm opened this issue 1 year ago • 6 comments

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:

  • KEY_METRICS_SELECTION_FORM - This is the list containing only the active/final selection, which is used to determine which metric items are in the checked state, and also for saving the metrics to the database.
  • KEY_METRICS_CURRENT_SELECTION_FORM - The current selection, which initially reflects the selection from the dashboard, and then aggregates all other metric items that were checked during the “panel session” transferred over from the list.
  • KEY_METRICS_STAGED_SELECTION_FORM - This list will be populated with active selection within the currently active “group panel session”, and will be transferred over to the KEY_METRICS_CURRENT_SELECTION_FORM list after that session is finished. And the list will be emptied, so it can be restarted for the next session.

This logic should reflect in ChipTabGroup component which should have all the groups and selection working correctly.

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

  • Metric items under Current selection group, in the selection panel are reflecting current selection consisting of both checked and unchecked items. To achieve this, they should be defined in 3 lists:
    • Effective selection metrics list
      • What is on the dashboard right now; from any source: tailored or saved choices. Also every other metric item that was selected from any other group while the selection panel is open.
    • Selected metrics list
      • What is currently checked "on" in the panel. This list contains only the the checked metrics, and will be used to check which item should be displayed as checked during each metric rendering - for all the groups including the Current selection one as well.
      • This logic is already implemented in the existing functionality of the selection panel, and is stored in the KEY_METRICS_SELECTION_FORM form key.
    • Unstaged 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 Unstaged list is reset (emptied).
  • Selection process adheres to the following guidelines:
    • Metrics included in the Effective selection are always present, regardless of checked state
    • Selected metrics are always included
    • Unchecking any metric should never cause it to immediately disappear from the focused list view
  • New logic is implemented with ChipTabGroup component, Current selection is populated correctly, and all metric items across the groups are reflecting the "checked" status if present in Current selection. At this stage the new selection panel layout (key metrics specific SelectionPanelItems implemented in 9378) is fully functional and included in the dashboard instead of SelectionPanelItems generic component when conversionReporting feature flag is enabled

Implementation Brief

  • [ ] In assets/js/components/KeyMetrics/MetricsSelectionPanel/index.js
    • Update onSideSheetOpen callback
      • Under the KEY_METRICS_SELECTION_FORM include additional form key EFFECTIVE_SELECTION that should also receive savedViewableMetrics for the initials items
  • [ ] Update assets/js/components/KeyMetrics/MetricsSelectionPanel/MetricItem.js
    • Following the current implementation of KEY_METRICS_SELECTION_FORM https://github.com/google/site-kit-wp/blob/b021e955ebf7bc3e95137e8f64c41ae6e9f3dce2/assets/js/components/KeyMetrics/MetricsSelectionPanel/MetricItem.js#L77-L83 add new form key UNSTAGED_SELECTION, using the same approach as existing KEY_METRICS_SELECTED, to store and filter only checked items. This should be conditionally done if the feature flag is enabled
  • [ ] Update assets/js/components/KeyMetrics/ChipTabGroup/index.js component
    • Update existing logic around Current selection to include all items from EFFECTIVE_SELECTION form key of KEY_METRICS_SELECTION_FORM form.
      • Since current selection group is dynamic, in case Current selection group is active, items should be listed from allMetricItems without group filter applied, but they should be filtered by their slug presence in EFFECTIVE_SELECTION list
    • In onClick callback where active group is switched, collect the items from UNSTAGED_SELECTION form, KEY_METRICS_SELECTED key and push them to the current array in EFFECTIVE_SELECTION form key
    • Reset the value for UNSTAGED_SELECTION form key to an empty array
    • selectedCount should be updated to collect the selected items in each group, by filtering out the items from activeMetricItems that have their slugs in KEY_METRICS_SELECTED form key, or if current selection group is selected, then items from EFFECTIVE_SELECTION that have their slugs present in KEY_METRICS_SELECTED
  • [ ] In assets/js/components/KeyMetrics/MetricsSelectionPanel/MetricItems.js
    • Conditionally render key metrics specific selection panel items assets/js/components/KeyMetrics/MetricsSelectionPanel/SelectionPanelItems.js component if feature flag is enabled, otherwise render the existing generic selection panel
      • Key metrics specific selection panel items component won't need existing props, except for savedItemSlugs

Test Coverage

  • Update any failing VRT/tests

QA Brief

Changelog entry

zutigrm avatar Sep 20 '24 14:09 zutigrm

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 :)

aaemnnosttv avatar Sep 30 '24 20:09 aaemnnosttv

@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

zutigrm avatar Oct 02 '24 15:10 zutigrm

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.

aaemnnosttv avatar Oct 09 '24 18:10 aaemnnosttv

@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 avatar Oct 10 '24 08:10 10upsimon

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

10upsimon avatar Oct 10 '24 08:10 10upsimon

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

zutigrm avatar Oct 24 '24 09:10 zutigrm

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

zutigrm avatar Nov 01 '24 16:11 zutigrm

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

10upsimon avatar Nov 04 '24 08:11 10upsimon

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

Notice how the 3rd tile is bigger than the first 2, when it's the same information being displayed.

Image

______________________________________________________________________________

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?

Other than that, most things have been verified good:

  • Current selection is reflecting the KMW from dashboard ✅

    Image

    Image

  • When unchecked, the items from Current selection group do not disappear but are unchecked. ✅

    https://github.com/user-attachments/assets/94840e56-374e-4ebc-99d1-bde4ba7b4649

  • For each group other than Current selection, the group is properly reflecting the number of selected items in that group. ✅

    https://github.com/user-attachments/assets/f3162b5c-5ac0-47f4-84f5-68904d6ad10d

  • Selecting items from Current selection also checks under their each respective group. Saving the metrics works like before. ✅

    https://github.com/user-attachments/assets/3db1b088-40b6-49ea-98e5-663e62540a35

  • Selecting less than 2 or more than 8 metrics throws an error. ✅

    https://github.com/user-attachments/assets/179dea1d-2276-4f2a-adaa-f62299996f16

  • On mobile, groups are horizontally scrollable. ✅

    https://github.com/user-attachments/assets/4c7e4bc2-cdb4-44d8-bc1b-af62d63d6d7d

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

ITEM 6 : Covered above in main issue. ✅

ITEM 7: Font is 14 px ✅

Image

ITEM 8: Font is 12px on mobile. ✅

Image

ITEM 9: Alignment is as expected. ✅

Image

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

kelvinballoo avatar Nov 25 '24 11:11 kelvinballoo

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

zutigrm avatar Nov 25 '24 13:11 zutigrm

QA Update ⚠

  • For ITEM E - Pending addition to the test site. ⚠

  • Noted on ITEM C. ✅

  • For ITEM D, a new ticket has been raised under https://github.com/google/site-kit-wp/issues/9776

  • ITEM A has been verified good. The clean slate is now working. ✅

    https://github.com/user-attachments/assets/ab1f9b32-a136-4e11-aac2-7dc03833f8c3

  • ITEM B has been verified good. Switching feature flag is not bringing an empty selection panel. ✅

    https://github.com/user-attachments/assets/79e3f55a-25b6-403a-a20c-e2305fb05b4f

Other than that, most things have been verified good:

  • Current selection is reflecting the KMW from dashboard ✅

    Image

    Image

  • When unchecked, the items from Current selection group do not disappear but are unchecked. ✅

    https://github.com/user-attachments/assets/94840e56-374e-4ebc-99d1-bde4ba7b4649

  • For each group other than Current selection, the group is properly reflecting the number of selected items in that group. ✅

    https://github.com/user-attachments/assets/f3162b5c-5ac0-47f4-84f5-68904d6ad10d

  • Selecting items from Current selection also checks under their each respective group. Saving the metrics works like before. ✅

    https://github.com/user-attachments/assets/3db1b088-40b6-49ea-98e5-663e62540a35

  • Selecting less than 2 or more than 8 metrics throws an error. ✅

    https://github.com/user-attachments/assets/179dea1d-2276-4f2a-adaa-f62299996f16

  • On mobile, groups are horizontally scrollable. ✅

    https://github.com/user-attachments/assets/4c7e4bc2-cdb4-44d8-bc1b-af62d63d6d7d

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

ITEM 6 : Covered above in main issue. ✅

ITEM 7: Font is 14 px ✅

Image

ITEM 8: Font is 12px on mobile. ✅

Image

ITEM 9: Alignment is as expected. ✅

Image

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

kelvinballoo avatar Nov 27 '24 07:11 kelvinballoo

QA Update ✅

Moving ticket to approval and everything is working as expected.

  • For ITEM E - The new groups 'Generating leads' and 'Selling products' are showing up when these events are detected and it's working as expected. ✅

    https://github.com/user-attachments/assets/703cf956-0d32-4a96-93b8-6ee39c0d1ae8

  • Noted on ITEM C. ✅

  • For ITEM D, a new ticket has been raised under https://github.com/google/site-kit-wp/issues/9776

  • ITEM A has been verified good. The clean slate is now working. ✅

    https://github.com/user-attachments/assets/ab1f9b32-a136-4e11-aac2-7dc03833f8c3

  • ITEM B has been verified good. Switching feature flag is not bringing an empty selection panel. ✅

    https://github.com/user-attachments/assets/79e3f55a-25b6-403a-a20c-e2305fb05b4f

Other than that, most things have been verified good:

  • Current selection is reflecting the KMW from dashboard ✅

    Image

    Image

  • When unchecked, the items from Current selection group do not disappear but are unchecked. ✅

    https://github.com/user-attachments/assets/94840e56-374e-4ebc-99d1-bde4ba7b4649

  • For each group other than Current selection, the group is properly reflecting the number of selected items in that group. ✅

    https://github.com/user-attachments/assets/f3162b5c-5ac0-47f4-84f5-68904d6ad10d

  • Selecting items from Current selection also checks under their each respective group. Saving the metrics works like before. ✅

    https://github.com/user-attachments/assets/3db1b088-40b6-49ea-98e5-663e62540a35

  • Selecting less than 2 or more than 8 metrics throws an error. ✅

    https://github.com/user-attachments/assets/179dea1d-2276-4f2a-adaa-f62299996f16

  • On mobile, groups are horizontally scrollable. ✅

    https://github.com/user-attachments/assets/4c7e4bc2-cdb4-44d8-bc1b-af62d63d6d7d

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

ITEM 6 : Covered above in main issue. ✅

ITEM 7: Font is 14 px ✅

Image

ITEM 8: Font is 12px on mobile. ✅

Image

ITEM 9: Alignment is as expected. ✅

Image

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

kelvinballoo avatar Nov 27 '24 10:11 kelvinballoo