Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 tasks
zutigrm opened this issue Sep 30, 2024 · 7 comments
Closed
2 tasks
Labels
P0 High priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@zutigrm
Copy link
Collaborator

zutigrm commented Sep 30, 2024

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

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
      <UserInputPreviewGroup
      slug={ USER_INPUT_QUESTIONS_PURPOSE }
      • 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

  • Set up Site Kit with tailored metrics (not user selected). Note the site purpose selected.
  • Navigate to Site Kit > Settings > Admin.
  • Within the Key Metrics settings group, edit the site purpose and change the value. Click Apply Changes.
  • The Tailored metric suggestions modal should display, showing current vs new metrics.
  • Dismissing the modal via the keep changes CTA should not change the site purpose, and it should remain as prior.
  • Updating the metric selection via the Update metric selection CTA should apply the changes, and the modal should reflect future changes made to the site purpose.
  • An example of the modal is shown in the screenshot below.

Image

Changelog entry

  • Add modal of tailored metrics to User Input Questionnaire.
@zutigrm zutigrm added P0 High priority Type: Enhancement Improvement of an existing feature Team S Issues for Squad 1 labels Sep 30, 2024
@zutigrm zutigrm self-assigned this Sep 30, 2024
@zutigrm zutigrm removed their assignment Sep 30, 2024
@10upsimon 10upsimon self-assigned this Sep 30, 2024
@10upsimon
Copy link
Collaborator

@zutigrm AC LGTM, moving to IB

@10upsimon 10upsimon removed their assignment Sep 30, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Oct 2, 2024
@eugene-manuilov eugene-manuilov self-assigned this Oct 4, 2024
@eugene-manuilov
Copy link
Collaborator

@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.

@zutigrm
Copy link
Collaborator Author

zutigrm commented Oct 10, 2024

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

@zutigrm zutigrm assigned eugene-manuilov and unassigned zutigrm Oct 10, 2024
@eugene-manuilov
Copy link
Collaborator

Thanks, IB ✔

@10upsimon
Copy link
Collaborator

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
Copy link
Collaborator

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 10upsimon assigned zutigrm and 10upsimon and unassigned 10upsimon and zutigrm Oct 31, 2024
@eugene-manuilov eugene-manuilov self-assigned this Nov 4, 2024
@eugene-manuilov eugene-manuilov removed their assignment Nov 4, 2024
@tofumatt tofumatt assigned tofumatt and 10upsimon and unassigned tofumatt Nov 6, 2024
@10upsimon 10upsimon assigned tofumatt and unassigned 10upsimon Nov 7, 2024
@tofumatt tofumatt removed their assignment Nov 7, 2024
@mohitwp mohitwp self-assigned this Nov 8, 2024
@mohitwp
Copy link
Collaborator

mohitwp commented Nov 11, 2024

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 Implement a new modal component showing current vs new tailored suggestions #9438.

Image

Image

Image

Recording.1571.mp4
Recording.1572.mp4

@mohitwp mohitwp removed their assignment Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants