-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Guided Tours: Add Jetpack Plugin Autoupdate Tour #25474
Conversation
0cc4a97
to
7e28953
Compare
8fa54a2
to
3ade47e
Compare
339baa0
to
d2fc725
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some implementation notes.
@@ -377,10 +381,10 @@ export default class Step extends Component { | |||
|
|||
const style = { ...this.props.style, ...stepCoords }; | |||
|
|||
return ( | |||
return ContentComponent ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't render and empty tour if no children are provided.
'Step#componentWillMount: stepSection: %s, name: %s', | ||
this.stepSection, | ||
this.props.name | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should rollback, I don't think Steps remount.
target=".plugin-item.is-placeholder" | ||
onTargetDisappear={ ( { next } ) => next() } | ||
next="onLoaded" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic, exhibit 1 (wait for placeholders to disappear)…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 Well played, Jon.
name="onLoaded" | ||
wait={ () => !! query( '.plugin-item-jetpack .form-toggle:enabled' ).length } | ||
target=".plugin-item-jetpack .form-toggle__switch" | ||
onTargetDisappear={ /** Errors if missing */ () => {} } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic, exhibit 2.
I think GT are buggy, while waiting it seems that the target appears and disappears. Ideally none of the logic would occur until wait passes.
Without onTargetDisappear
, this errors as the component appears.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to add a code comment above this whole step, too.
/cc @lsinger for GT bugginess
! isAutomatedTransfer && <ChecklistShow /> } | ||
! isAutomatedTransfer && ( | ||
<Fragment> | ||
<QueryJetpackPlugins siteIds={ [ selectedSiteId ] } /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query 'em early. As long as this query finishes, the guided tour should display immediately.
) } | ||
</p> | ||
<ButtonRow> | ||
<SiteLink isButton href={ '/plans/my-plan/:site' }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no {
<Step | ||
name="onLoaded" | ||
wait={ () => !! query( '.plugin-item-jetpack .form-toggle:enabled' ).length } | ||
target=".plugin-item-jetpack .form-toggle__switch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the 'plugin-item-' + plugin.slug
class to plugin-item.jsx
(see below) so the GT could easily query the JP plugin toggle. However, there's already an id
we might be able to use
Specifically, we might use label[for=autoupdates-jetpack-:site] .form-toggle__switch
as selector (which means adding some more silly :site
replacements to GT -- there's precedent).
Not sure it's worth it 🤷♂️
@@ -343,7 +343,9 @@ class PluginItem extends Component { | |||
pluginActions = this.renderActions(); | |||
} | |||
|
|||
const pluginItemClasses = classNames( 'plugin-item', { disabled } ); | |||
const pluginItemClasses = classNames( 'plugin-item', 'plugin-item-' + plugin.slug, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aligns with #26043
5291247
to
6a7a07a
Compare
Thanks @joanrho, I can confirm a positioning issue. I'll try to determine whether that's introduced by this PR or an existing bug. If you expand your viewport a bit, it should be on the right toggle. A workaround would be to change the popover so the box is positioned on the other side and the sidebar provides some room. |
@sirreal Yes, let's merge and move forward for now, since the Issue has already been captured. Thanks! |
Adds the tour.
Waits for plugins to be loaded with some magic. See notes. Existing tools don't seem to be applicable because plugin store is not redux and the wait may be too long.
Try rolling back f7170ea01ee3b09c0f8392ad6aab6074e3bb4102 while testing.
Ensure you test with clean application state and with already loaded plugin data in state. Should display the tour under all conditions.
Testing
Visit the checklist for a new Jetpack site and click the plugin auto-update step.
Closes #25458