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.22k stars 277 forks source link

Implement RRM disconnection confirmation modal #8845

Closed nfmohit closed 1 day ago

nfmohit commented 1 month ago

Feature Description

The module disconnection confirmation modal should be updated for Reader Revenue Manager to include a warning to let the user know that:


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

Acceptance criteria

Implementation Brief

  1. In assets/js/modules/reader-revenue-manager/index.js, add the following in features array in registerModule function.
    __(
        'Reader Revenue Manager publication tracking (your Reader Revenue Manager account will still remain active)',
        'google-site-kit'
    ),

Adding this would add the intended message in the modal.

  1. Add a new css rule so that the feature text is visible completely. The rule can be like
    .googlesitekit-settings-module--reader-revenue-manager .mdc-list-item__text {
        white-space: normal;
    }

This rule can be added to assets/sass/vendor/_mdc-dialog.scss.

Test Coverage

  1. Add a test that features for the module is available and the text should match which is in AC.

QA Brief

Note: Although the module cannot be setup completely as of now, but this issue can be tested with the given steps below.

Changelog entry

nfmohit commented 1 month ago

Hi @sigal-teller 👋 Would you be so kind as to help us with the UX here? For example, here is a module disconnection banner for the Analytics module:

image

This is a common component that we use across all modules with just the module name and list of features changing between modules, so I don't think we'd want to change this too much. For the Reader Revenue Manager module, there is a requirement to let the user know at this stage the following:

  • Disconnecting the module will only remove the snippet added by Site Kit, but will not deactivate Reader Revenue Manager.
  • They can visit the Reader Revenue Manager platform if they intend to deactivate it.

In my opinion, a small notice before the disconnect action should be sufficient. Thank you!

sigal-teller commented 3 weeks ago

@nfmohit I added the UX for the disconnect module here with one comment for you to review. Thank you!

nfmohit commented 3 weeks ago

LGTM, thank you @sigal-teller !

kuasha420 commented 2 weeks ago

IB :white_check_mark:

wpdarren commented 5 days ago

QA Update: ❌

@ankitrox I have a very minor UX/UI issue that I would like to highlight. The bulletpoint and subsequent paragraph are not aligned as per the figma designs. There needs to be some padding/margin.

Test site: image

Figma design image

ankitrox commented 4 days ago

I have added couple of lines of CSS fix to accommodate the design change. Please note that those bullets will be aligned in a similar fashion (as of RRM disconnection modal) for other disconnection modal as well; for example: Analytics-4 disconnection modal.

I have to added the screenshots for the same.

Screenshot 2024-07-22 at 9.14.10 PM.png Screenshot 2024-07-22 at 9.14.29 PM.png Screenshot 2024-07-22 at 9.15.28 PM.png Screenshot 2024-07-22 at 9.22.13 PM.png
techanvil commented 4 days ago

Thanks @kuasha420, however, the proposed fix does introduce a regression, as the bullet points and feature descriptions are no longer centre-aligned vertically. For example this is how the Analytics modal currently looks, and the fix we introduce should retain this styling while ensuring the bullet point is correctly aligned when a feature description spans multiple lines of text.

image

I've taken a look into it myself and can see that, while fairly trivial overall, the fix could be relatively time consuming compared to the severity of the defect.

I would therefore suggest we treat this as a known issue to address as part of the bug bash or post-launch. @wpdarren, could you please create an issue to track it? We can prioritise is as a P2 for the time being and reassess at bug bash time.

wpdarren commented 4 days ago

QA Update: ✅

Verified:

Note: that is a styling issue where the bullet point is not aligned to the text like the figma designs and in addition the horizontal line has no space between the text underneath.

I will create a ticket for this since it should be fixed as it look odd.

image