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.25k stars 291 forks source link

Improve rendering time of newly refactored "Setup Success" subtle notifications #9488

Open jimmymadon opened 3 weeks ago

jimmymadon commented 3 weeks ago

Feature Description

As per this comment thread, there is an increased lag when loading the newly refactored RRM Setup Success Subtle Notification compared to the legacy implementation. This could be because refactored Subtle Notifications are now not rendered immediately simultaneously in the React DOM. They are evaluated along with all other registered Error and Banner notifications and only after all the checkRequirements callbacks are run, then the topmost notification in the queue is rendered.

One additional way of solving this would be to introduce the concept of "ad-hoc" notifications which are registered and added to the queue instantaneously without the entire queue of notifications being (re)evaluated. Issue #9453 has been created to explore this possibility. Hence that issue will block this one to ensure we don't create multiple solutions unnecessarily.


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

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

eugene-manuilov commented 3 weeks ago

Setup success subtle notifications within the plugin should be rendered without any delay on page load after the setup of the feature they notify about is completed.

@jimmymadon I don't think we should do it this way. I think we need to reconsider our main resolver to be split into area specific resolvers to make sure subtle notifications don't need to wait for other notification checks to be resolved.

jimmymadon commented 3 weeks ago

@eugene-manuilov Yes - sounds good to me. That was the original problem's solution - the only reason I mentioned the "ad-hoc" approach is because that is being investigated in #9453 as well.

Anyway, the AC here would not change though - as it is generic and just says that there should be no delay between us doing a setup and the notification being displayed.

eugene-manuilov commented 3 weeks ago

Yeah, makes sense. Thanks, @jimmymadon. AC ✔

techanvil commented 1 day ago

@aaemnnosttv @jimmymadon, I had the POC branch for this checked out while investigating another issue (https://github.com/google/site-kit-wp/issues/9261) and noticed a couple of issues that needed to be fixed. Evan I hope you don't mind, I've pushed the fix directly to the branch, see https://github.com/google/site-kit-wp/commit/5331d191d21105347febe19362d24c988c701ece.

The tests will of also need updating to reflect the change, but that's something that can no doubt be tackled during implementation.