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

Implement a new modal component showing current vs new tailored suggestions

Open zutigrm opened this issue 1 year ago • 4 comments

Feature Description

Once the site purpose question has been updated for tailored metrics users and the user has clicked the “Confirm changes” CTA, a new modal component will render, displaying the current metrics and the new tailored metrics alongside one another.

FIgma designs

See New modal component to update the tailored metrics based on the changed answer section in the design doc


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

Acceptance criteria

  • A new modal component is added matching the Figma design
  • The Current metrics list will show metric items currently displayed on the dashboard
  • The New tailored metrics list will show the metric items that will be used instead, based on the updated site purpose answer
  • Clicking the Keep current selection CTA will only close the modal
  • Clicking the Update metrics selection CTA should apply the change and re-compute the tailored metrics based on the updated site purpose answer.
  • See the New modal component to update the tailored metrics based on the changed answer section in the design doc for a more technical overview
  • This issue should only add new component and accompanying stories, trigger of the modal based on the answer update will be handled in #9439

Implementation Brief

  • [x] Add assets/js/components/KeyMetrics/ConfirmSitePurposeChangeModal.js
    • Use assets/js/components/ModalDialog.js as a starting point
    • dialogActive and onClose props will be used to control opening/closing of the dialog
    • Add the title and description from Figma design
    • Update DialogContent to show 2 column layout as per design
    • For the Current metrics, retrieve the items using getKeyMetrics selector from CORE_USER datastore and render them in a list
    • For the New tailored metrics, you can use getAnswerBasedMetrics selector. But modify it accept extra param for purpose value. And then allow purpose to be overridden when included https://github.com/google/site-kit-wp/blob/9a1a4f1d66be97d1839e663b4c3b4eceb839d081/assets/js/googlesitekit/datastore/user/key-metrics.js#L266
      • Retrieve the settings from getUserInputSettings selector and extract the purpose value like in the referenced code above. The new selected answer will be updated in the state upon changing, so it will reflect how tailored metrics will look like when passed as an argument to the getAnswerBasedMetrics selector
    • First CTA should have label Keep current selection
      • It should invoke onClose callback prop
    • Second CTA should have label Update metrics selection
      • As a callback when this button is clicked, invoke saveUserInputSettings action from the CORE_USER datastore, and close the modal

Test Coverage

  • Add stories for ConfirmSitePurposeChangeModal component
  • Update assets/js/googlesitekit/datastore/user/key-metrics.test.js to include a test case for getAnswerBasedMetrics when purpose argument is included

QA Brief (QA:ENG)

  • Set up Site Kit with tailored KMW with an option other than Publish News
  • The following sections will implement the modal in a forced visible mode in order to test it:
    • Within the SettingsCardKeyMetrics component:
      • Import useState in the existing import line for '@wordpress/element'
      • Within the SettingsCardKeyMetrics component:
        • Add a default state with a setter for dialog visibility: const [ dialogActive, toggleDialog ] = useState( true );
        • Add a handleDialog callback function to handle closing of the modal:
	const handleDialog = useCallback( () => {
		toggleDialog( ! dialogActive );
	}, [ dialogActive ] );
    * Between the `isUserInputCompleted` and `isUserInputCompleted === false` statement blocks, output the modal for testing:
{ isUserInputCompleted && (
    <ConfirmSitePurposeChangeModal
        dialogActive={ dialogActive }
        handleDialog={ handleDialog }
    />
) }
     * This should trigger the modal on admin settings page load.
     * Confirm that the current and new KMW items are the same in the modal (as no change to site purpose has been made), as per the screenshot below:

Image

    * Now, within the `ConfirmSitePurposeChangeModal` mock an update for site purpose by forcing the value of the `purpose` const to be `'publish_news'` as opposed to getting the value from the store.
    * Reload the page and validate that the current vs new metrics lists are now different as per the image below:

Image

  • Validate that unit tests are passing.

Changelog entry

zutigrm avatar Sep 30 '24 10:09 zutigrm

@zutigrm AC approved, thank you

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

  • For dialogActive value, leverage getValue selector from CORE_UI datastore, and pull value from, say KEY_METRICS_CONFIRM_SITE_PURPOSE_CHANGE_OPEN key ...
  • Callback function will close the modal updating the KEY_METRICS_CONFIRM_SITE_PURPOSE_CHANGE_OPEN to false. You can see an example usage here

@zutigrm, this component should always render the dialog whenever the component is rendered itself. In other words, it should not determine its state from the CORE_UI value. The parent component will have to make a decision whether this dialog should be displayed or not. To close the dialog, the parent component should pass an appropriate onClose callback.

This will make this component less coupled to the current state and it will be easier for us to maintain it and its stories.

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

Thanks @eugene-manuilov IB updated, reference to the UI state has been removed, the default props will control the dialog state

zutigrm avatar Oct 10 '24 09:10 zutigrm

Thanks, IB ✔

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

QA Update ⚠

Note : For QA:Eng. I was testing the https://github.com/google/site-kit-wp/issues/9439 and verified the figma design as mentioned in AC of this ticket. I added couple of observations related to it below.

@10upsimon

1) In Figma title font size is 28px where as under actual implementation it is 22px.

Image

Image

2) Not a major issue, but I want to point out that on mobile devices, the modal title is slightly cut off.

Image

Image

mohitwp avatar Nov 11 '24 04:11 mohitwp

QA Update ✅

  • Tested on main environment.
  • Verified logic and functionality under #9439
  • Verified issue reported above is resolve now and tile font size is updated to 28px.

Image

Image

mohitwp avatar Nov 14 '24 09:11 mohitwp