site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Add logic to trigger modal once user input questions have been updated

Open zutigrm opened this issue 1 year ago • 4 comments

Feature Description

Following the implementation of new modal in #9438 this issue should connect the trigger for that modal, once site purpose question is changed in the Key metrics admin settings. Once the save button is clicked, and answer was modified, modal should be triggered


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

Acceptance criteria

  • Once the site purpose question is changed by the user in the Key metrics admin settings and the save button is clicked, it should open the new modal implemented in the #9438.
    • Modal should be triggered only when conversionReporting feature flag is enabled

Implementation Brief

  • [ ] In assets/js/components/user-input/UserInputPreviewGroup.js
    • Include new prop onChange
    • Update submitChanges callback, invoke onChange prop if present within the if( ! response.error ) conditional check
  • [ ] Update assets/js/components/user-input/UserInputPreview.js
    • Add local state [isModalOpen, setIsModalOpen] for example, with default value of false
    • Include onChange prop to the https://github.com/google/site-kit-wp/blob/14f36cc292f05d0e72213ff63277e3873d89c7a1/assets/js/components/user-input/UserInputPreview.js#L145-L146
      • In the callback, return early if conversionReporting feature flag is not enabled, otherwise set a isModalOpen local state to true
    • Include ConfirmSitePurposeChangeModal component, wrapped within Portal component, so it is appended in the body outside the settings container.
      • Pass the isModalOpen value to the dialogActive
      • For onClose prop, use callback that resets isModalOpen to false

Test Coverage

  • No update needed

QA Brief

Changelog entry

zutigrm avatar Sep 30 '24 11:09 zutigrm

@zutigrm AC LGTM, moving to IB

10upsimon avatar Sep 30 '24 15:09 10upsimon

@zutigrm, I think it would be better to add a new prop onChange to the UserInputPreviewGroup component that will let us execute a callback when the setting is changed. Then use this prop for the corresponding question and display the new modal dialog in the UserInputPreview component without dealing with the CORE_UI state.

eugene-manuilov avatar Oct 05 '24 10:10 eugene-manuilov

@eugene-manuilov makes sense, thanks. I updated IB to reflect this approach

zutigrm avatar Oct 10 '24 09:10 zutigrm

Thanks, IB ✔

eugene-manuilov avatar Oct 11 '24 09:10 eugene-manuilov

Adding an update following internal sync and comms with @zutigrm:

The logic needed to be tweaked as the current user input options are saved to the user input settings store, they're just not persisted. This affects the modal logic (specifically due to how we retrieve the user input values) as the old vs new would always be the same in this case.

The solution was to introduce a temporary form and set the old/current site purpose. This value is in turn used in the modal to get the current values against the existing purpose slug stored in the temporary form store. The temporary form is reset on modal save so the logic can be repeated for future changes.

10upsimon avatar Oct 31 '24 07:10 10upsimon

Flagging further that additional logic needed to be introduced to the modal that resets the user input settings site purpose value to the prior purpose if the user dismisses the metric updates in the modal.

10upsimon avatar Oct 31 '24 07:10 10upsimon

QA Update ✅

  • Tested on dev environment.
  • Tested when 'conversionReporting' feature flag disabled and verified that Modal not triggered only when conversionReporting feature flag is disabled.
  • Tested when 'conversionReporting' feature flag enabled and verified that Modal triggered when conversionReporting feature flag is enabled.
  • Verified that clicking on 'Keep current selection' CTA or outside the modal do not change the purpose of the site.
  • Verified that clicking on 'Update metrics selection' change the purpose and metrics selection as per selected purpose on main dashboard.
  • Verified that once the site purpose question is changed by the user in the Key metrics admin settings and the save button is clicked, it open the new modal implemented in the https://github.com/google/site-kit-wp/issues/9438.

Image

Image

Image

https://github.com/user-attachments/assets/6ffa1ee9-46fe-4eba-94ce-d5b6dc7e6794

https://github.com/user-attachments/assets/b040ed40-5d0e-4542-abec-6d7fa4ae1ceb

mohitwp avatar Nov 11 '24 04:11 mohitwp