-
Notifications
You must be signed in to change notification settings - Fork 295
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
Make the plugin activation banner a simple CTA link instead of replicating the splash screen #4403
Comments
@techanvil we shouldn't delete those divs because they create the layout for the banner. Let's leave it but replace them with the appropriate |
@eugene-manuilov, I do believe these divs are redundant in practical terms, i.e. they don't actually add any useful layout in this particular case (if you look into it, it's just creating a single spanned grid cell, which is essentially redundant). Nevertheless it seems reasonable to leave the logical grid structure in place, and I've updated the IB accordingly to replace with |
Thanks, @techanvil! IB ✔️ |
@kuasha420 could you please add QAB? |
QA Update: ✅Verified:
|
We've recently re-evaluated the behavior of the plugin activation banner: Somewhat of a problem with that is that it is a duplicated version of the plugin splash screen, however with simplified UI. This comes with problems like the following:
The only benefit of what the activation banner currently does is that it avoids a single extra click for the case that an admin activates the plugin and immediately wants to set it up. However, you could still argue that even in that case leading the user to the splash screen instead will eventually provide better UX since there is a central place to start the setup.
Related is #4343 (comment), which led us to make a decision here more promptly - before we had already been thinking about this at a high level, but that issue shows it's already becoming a problem.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
googlesitekit-dashboard
).googlesitekit-splash
).view_notification
andconfirm_notification
should be removed (since they are no longer relevant with the new behavior).confirm_notification
GA event should still fire whenever the button-like link is clicked, and it should be updated to include an event label: If the link is to the dashboard, the label should bedashboard
, otherwise it should besplash
.Implementation Brief
Within
assets/js/components/activation/activation-app.js
:buttonURL
todashboardURL
orsplashURL
, depending on the value ofcanViewDashboard
.onButtonClick
callback:confirm_notification
to include the labeldashboard
orsplash
, depending on the value ofcanViewDashboard
.trackEvent
calls.<Fragment>
and<NotificationCounter />
from the returned JSX.Within
assets/js/components/activation/activation-main.js
:<Button>
directly below the<h3>
.disabled
prop from the<Button>
.<CompatibilityChecks>
component including all descendants.div
s with classesmdc-layout-grid
,mdc-layout-grid__inner
andmdc-layout-grid__cell
can be replaced withGrid
,Row
andCell
components.Merge the above two files:
activation-app.js
with that ofactivation-main.js
, and deletingactivation-main.js
.Within
assets/sass/components/activation/_googlesitekit-activation.scss
:.googlesitekit-activation
:.googlesitekit-activation__title
, setting the bottom margin to20px
to add space between the header and CTA.googlesitekit-activation__text
andgooglesitekit-opt-in
.Test Coverage
QA Brief
Changelog entry
The text was updated successfully, but these errors were encountered: