-
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
Themes: 'Thanks' modal on activation from Theme Sheet #5259
Conversation
b60d83e
to
07cb64f
Compare
07cb64f
to
c6647b3
Compare
There already is an |
Yeah, good point, and I thought about this a bit at the time. In the end I wasn't totally happy with the tests and production code sharing the same empty component, since it seems they serve different purposes. Hopefully the test version will be longer lived than this production one. |
@@ -1,3 +1,5 @@ | |||
/* eslint-disable react/jsx-no-bind */ |
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.
Please don't unless there's a way more compelling reason than elsewhere :-)
What about changing the trackClick
method as follows instead:
boundTrackClick: function( eventName ) {
return Helpers.trackClick.bind( null, 'current theme', eventName );
}
That should allow usage like
onClick={ this.boundTrackClick( 'features' )}
Yeah. :-/ Alternatively, tests could also use the production variant. But I guess I'm fine with the dupe for now since it's such a small component. |
<ThanksModal | ||
site={ this.props.selectedSite } | ||
source={ 'details' } | ||
clearActivated={ bindActionCreators( clearActivated, this.props.dispatch ) }/> |
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.
Consider using the second argument to the connect
call, mapDispatchToProps
, instead:
export default connect(
( state ) => {
/* ... */
},
{ clearActivated }
)( ThemeSheet );
(and just use clearActivated={ this.props.clearActivated }
then -- could do the same with the other actions you dispatch
, BTW.)
Noting here that with premium themes, the flow works fine, but I end up at
|
Left a couple of nitpicks. Looks good overall, and tests well! |
Regarding the
I disagree here, because this component will never be on a hot path, so it's not worth obfuscating code to prevent the eslint warning. I have moved to your suggestion because the |
* Themes: Thanks modal in sheet when activating theme * Add README.md for empty component * Themes: `source` prop for thanks modal * Themes: Show customize link in modal on sheet activation * Themes: Click-tracking for thanks-modal * Themes: use mapDispatchToProps for sheet actions
Acknowledge activation of a theme from the theme sheet with the existing 'Thanks' modal dialog.
Currently only works when a site is selected. A site selector for the sheet is coming soon.
To Test
Expected: The modal should display
Expected: The first link in the modal should be to the customizer