Jetpack Settings: Add notices for (de)activating Jetpack modules #10583

tyxla · 2017-01-12T13:21:12Z

Purpose

This PR introduces a universal handler for dispatching notices upon activation and deactivation of Jetpack modules. This will allow us to configure specific notices on a per-module basis when activating/deactivating modules, but it will still default to a generic message if we haven't configured specific notices for a certain module. This PR is part of #9171.

Advantages and further configuration

This approach will allow easy configuration of the notices on a per-module basis - if one wishes to specify notices for a certain module, all that needs to be done is to add the following snippet to the constants.js file:

'module-slug': {
	[ ACTIVATE_SUCCESS ]: translate( 'Message when activation was successful.' ),
	[ DEACTIVATE_SUCCESS ]: translate( 'Message when deactivation was successful.' ),
	[ ACTIVATE_FAILURE ]: translate( 'Message when activation was not successful.' ),
	[ DEACTIVATE_FAILURE ]: translate( 'Message when deactivation was not successful.' ),
},

where there are 5 strings to configure:

module-name - the slug of the module - for example infinite-scroll, minileven, etc.
4 separate strings for each of the combinations (success/failure on activation/deactivation).

This PR adds an example set of specific notices for the infinite-scroll module. We can use them as basis for creating other specific notices.

Preview

Successful activation of module with specific notices

Error during activation of module with specific notices

Successful activation of module without specific notices

Error during deactivation of module without specific notices

To test

Checkout this branch, or get it going on calypso.live
Load any Calypso page.
Open the development console in your browser
Run the following in your console:
- To test the handler for module with specific notices:
  - dispatch( { type: 'JETPACK_MODULE_ACTIVATE_SUCCESS', moduleSlug: 'infinite-scroll' } )
  - dispatch( { type: 'JETPACK_MODULE_ACTIVATE_FAILURE', moduleSlug: 'infinite-scroll' } )
  - dispatch( { type: 'JETPACK_MODULE_DEACTIVATE_SUCCESS', moduleSlug: 'infinite-scroll' } )
  - dispatch( { type: 'JETPACK_MODULE_DEACTIVATE_FAILURE', moduleSlug: 'infinite-scroll' } )
- To test the handler for a module without specific notices:
  - dispatch( { type: 'JETPACK_MODULE_ACTIVATE_SUCCESS', moduleSlug: 'minileven' } )
  - dispatch( { type: 'JETPACK_MODULE_ACTIVATE_FAILURE', moduleSlug: 'minileven' } )
  - dispatch( { type: 'JETPACK_MODULE_DEACTIVATE_SUCCESS', moduleSlug: 'minileven' } )
  - dispatch( { type: 'JETPACK_MODULE_DEACTIVATE_FAILURE', moduleSlug: 'minileven' } )
Verify that all notice messages and their colors make sense.

matticbot · 2017-01-12T13:21:13Z

Test live: https://calypso.live/?branch=add/jetpack-module-activation-notices

tyxla · 2017-01-12T13:23:27Z

/cc @MichaelArestad - we could use some feedback here for:

Text of notices that we've used in this PR - do you have any suggestions on how we can improve them?
The proposed way of adding new custom notices - does it seem simple enough and easy to add new ones without any help from developers?

MichaelArestad · 2017-01-12T16:17:26Z

It seems simple enough to spot and modify.

'Infinite scroll has been enabled.'
'Infinite scroll has been disabled.'

Is there any way we can make these messages more helpful? How does a user fix it?
'Infinite scroll could not be enabled.'
'Infinite scroll could not be disabled.'

Might want an additional one for themes that don't support it (I think we may already do this in JP) CC @jeffgolenski
"Infinite scroll support has been enabled, but your theme doesn't support it." button: Learn more

tyxla · 2017-01-13T09:42:51Z

@MichaelArestad thank you for the feedback!

Is there any way we can make these messages more helpful? How does a user fix it?

Yes, of course! However, my plan for this PR was to lay the ground work for these notices. Then, in subsequent PRs we can start building more helpful and informative notices on a per-module basis. Does that sound like a good plan?

roccotripaldi

Code looks good.
This seems to be a solid baseline for activation / deactivation notices. I like how we have defaults in place, as well as the option to add specific notices on a per module basis.

To address Michael's feedback, I think we can work on writing more fine tuned notices in future PRs.

Thanks to Jetpack's awesome sync, we are syncing theme support options:
https://github.com/Automattic/jetpack/blob/master/sync/class.jetpack-sync-defaults.php#L258https://github.com/Automattic/jetpack/blob/master/sync/class.jetpack-sync-defaults.php#L258

So in theory, we can do exactly as Michael says for infinite scroll, and let folks in calypso know that their theme does not support it.

If you are fine with this as a baseline @MichaelArestad - can you mark it as "Ready to Merge" ?

tyxla · 2017-01-25T08:03:44Z

Thanks for the feedback @roccotripaldi!

@MichaelArestad can we get your feedback on this one? We've already deployed some cards, so I feel these notices are very necessary.

MichaelArestad · 2017-01-25T23:38:06Z

I think it's okay. I'd like a second opinion on the defaults. Maybe @ashleighaxios can weigh in.

ashleighaxios · 2017-01-27T18:30:14Z

I might adjust the language since the double negative is tricky (could not be disabled).

Success:
'Infinite scroll is now on.'
Alt to @MichaelArestad's point: 'Infinite scroll is now on, but not supported by your theme.button: Learn more'
'Infinite scroll is now off.'

Failure:
'Infinite scroll could not be switched on.'
'Infinite scroll could not be switched off.'

Alternative #2: Rather than allowing infinite scroll to turn on when it won't work, could we fail gracefully there? IE: 'Infinite scroll is not supported by your theme and could not be switched on. button: Learn more' I'm fine with the other way, but, being nit picky, think it may be confusing when the confirmation message goes away; it will look like infinite scroll is on without having the feature realized.

a8ci18n · 2017-01-30T11:24:29Z

client/state/notices/jetpack-modules/index.js

+			messageType = 'error';
+			break;
+		case JETPACK_MODULE_DEACTIVATE_FAILURE:
+			message = message || translate( 'An error occurred during deactivation.' );


Alternate string suggestion:

translate( 'An error occurred while deactivating %(plugin)s on %(site)s.' )— translations: 39. ES Score: 4.59

PR translation status page

a8ci18n · 2017-01-30T11:24:30Z

client/state/notices/jetpack-modules/index.js

+			messageType = 'success';
+			break;
+		case JETPACK_MODULE_ACTIVATE_FAILURE:
+			message = message || translate( 'An error occurred during activation.' );


Alternate string suggestion:

translate( 'An error occurred when activating %(plugin)s.' )— translations: 19. ES Score: 4.92

PR translation status page

a8ci18n · 2017-01-30T11:24:30Z

client/state/notices/jetpack-modules/index.js

+			messageType = 'success';
+			break;
+		case JETPACK_MODULE_DEACTIVATE_SUCCESS:
+			message = message || translate( 'Deactivated successfully.' );


Alternate string suggestion:

translate( 'Successfully deactivated %(plugin)s on %(site)s.' )— translations: 21. ES Score: 9.84

PR translation status page

a8ci18n · 2017-01-30T11:24:30Z

client/state/notices/jetpack-modules/index.js

+
+	switch ( type ) {
+		case JETPACK_MODULE_ACTIVATE_SUCCESS:
+			message = message || translate( 'Activated successfully.' );


Alternate string suggestion:

translate( 'Successfully activated %(plugin)s on %(site)s.' )— translations: 21. ES Score: 8.76

PR translation status page

tyxla · 2017-01-30T11:27:23Z

@ashleighaxios I've updated the Infinite Scroll notices to the ones you recommended.

However, I think @MichaelArestad requested a second opinion on the default notices, which we're trying to keep non-specific and as universal as possible, because they'll be used for any module that does not have a specific message. My initial suggestions were:

successful activation: Activated successfully.
successful deactivation: Deactivated successfully.
error during activation: An error occurred during activation.
error during deactivation: An error occurred during deactivation.

@ashleighaxios do you have any recommendations for improving these?

a8ci18n · 2017-01-30T11:34:44Z

client/state/notices/jetpack-modules/constants.js

+export const MODULE_NOTICES = {
+	'infinite-scroll': {
+		[ ACTIVATE_SUCCESS ]: translate( 'Infinite scroll is now on.' ),
+		[ DEACTIVATE_SUCCESS ]: translate( 'Infinite scroll is now off.' ),


Alternate string suggestion:

translate( 'Scroll Infinitely' )— translations: 25. ES Score: 4.14

PR translation status page

a8ci18n · 2017-01-30T11:34:44Z

client/state/notices/jetpack-modules/constants.js

+
+export const MODULE_NOTICES = {
+	'infinite-scroll': {
+		[ ACTIVATE_SUCCESS ]: translate( 'Infinite scroll is now on.' ),


Alternate string suggestion:

translate( 'Scroll Infinitely' )— translations: 25. ES Score: 4.27

PR translation status page

tyxla · 2017-02-01T10:09:42Z

@MichaelArestad @ashleighaxios we already have several cards in the wild, so IMO it would be great if we could release some notices upon activation/deactivation of modules, just so users know that this is immediate and does not require saving. Do you folks feel there's anything that prevents this from landing? We could always improve these and build more granular notices once we have the infrastructure for that.

ashleighaxios · 2017-02-01T15:29:24Z

@tyxla @MichaelArestad No objections from me. The new language works, what you're saying makes sense, and we're happy to adjust further down the road.

MichaelArestad · 2017-02-01T15:35:11Z

@tyxla We could just match the language to @ashleighaxios's previous suggestions.

"Turned on successfully"
"Turned off successfully"

The error messages seem just fine, but hopefully we have specific errors for everything.

tyxla · 2017-02-02T09:13:42Z

@MichaelArestad @ashleighaxios thanks for the suggestions! I've improved the messages as you suggested, merging now. I'd be happy to implement better, more granular messages for modules in the future, so if you have any ideas, shoot away 😃

tyxla added Jetpack [Feature] Site Settings All other general site settings. State [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 12, 2017

tyxla added this to the Jetpack Module Settings in Calypso milestone Jan 12, 2017

tyxla self-assigned this Jan 12, 2017

tyxla requested review from ryelle, oskosk, johnHackworth and roccotripaldi January 12, 2017 13:21

tyxla mentioned this pull request Jan 12, 2017

Jetpack Settings: Add Theme Enhancements card under Writing Settings for Jetpack sites #10083

Merged

oskosk mentioned this pull request Jan 12, 2017

Implement Jetpack Module Settings #9171

Closed

55 tasks

roccotripaldi approved these changes Jan 23, 2017

View reviewed changes

roccotripaldi added [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 23, 2017

tyxla force-pushed the add/jetpack-module-activation-notices branch 2 times, most recently from 34adfab to 69f348e Compare January 30, 2017 11:19

a8ci18n reviewed Jan 30, 2017

View reviewed changes

tyxla added 4 commits February 2, 2017 11:03

Notices: Add notice handler and config for Jetpack module activation

a7481c3

Notices: Add Jetpack module activation notices to middleware

db9de09

Notices: Update (de)activation messages for Jetpack modules

2038605

Notices: Update default (de)activation messages for Jetpack modules

b8ae80f

tyxla force-pushed the add/jetpack-module-activation-notices branch from 48dfbfd to b8ae80f Compare February 2, 2017 09:06

tyxla added [Status] Ready to Merge and removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Feb 2, 2017

tyxla merged commit fe09df5 into master Feb 2, 2017

tyxla deleted the add/jetpack-module-activation-notices branch February 2, 2017 09:16

tyxla removed the [Status] Ready to Merge label Feb 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jetpack Settings: Add notices for (de)activating Jetpack modules #10583

Jetpack Settings: Add notices for (de)activating Jetpack modules #10583

tyxla commented Jan 12, 2017 •

edited

Loading

matticbot commented Jan 12, 2017

tyxla commented Jan 12, 2017

MichaelArestad commented Jan 12, 2017

tyxla commented Jan 13, 2017

roccotripaldi left a comment •

edited

Loading

tyxla commented Jan 25, 2017

MichaelArestad commented Jan 25, 2017

ashleighaxios commented Jan 27, 2017

a8ci18n Jan 30, 2017

a8ci18n Jan 30, 2017

a8ci18n Jan 30, 2017

a8ci18n Jan 30, 2017 •

edited

Loading

tyxla commented Jan 30, 2017

a8ci18n Jan 30, 2017

a8ci18n Jan 30, 2017

tyxla commented Feb 1, 2017

ashleighaxios commented Feb 1, 2017

MichaelArestad commented Feb 1, 2017 •

edited

Loading

tyxla commented Feb 2, 2017

Jetpack Settings: Add notices for (de)activating Jetpack modules #10583

Jetpack Settings: Add notices for (de)activating Jetpack modules #10583

Conversation

tyxla commented Jan 12, 2017 • edited Loading

Purpose

Advantages and further configuration

Preview

Successful activation of module with specific notices

Error during activation of module with specific notices

Successful activation of module without specific notices

Error during deactivation of module without specific notices

To test

matticbot commented Jan 12, 2017

tyxla commented Jan 12, 2017

MichaelArestad commented Jan 12, 2017

tyxla commented Jan 13, 2017

roccotripaldi left a comment • edited Loading

Choose a reason for hiding this comment

tyxla commented Jan 25, 2017

MichaelArestad commented Jan 25, 2017

ashleighaxios commented Jan 27, 2017

a8ci18n Jan 30, 2017

Choose a reason for hiding this comment

a8ci18n Jan 30, 2017

Choose a reason for hiding this comment

a8ci18n Jan 30, 2017

Choose a reason for hiding this comment

a8ci18n Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

tyxla commented Jan 30, 2017

a8ci18n Jan 30, 2017

Choose a reason for hiding this comment

a8ci18n Jan 30, 2017

Choose a reason for hiding this comment

tyxla commented Feb 1, 2017

ashleighaxios commented Feb 1, 2017

MichaelArestad commented Feb 1, 2017 • edited Loading

tyxla commented Feb 2, 2017

tyxla commented Jan 12, 2017 •

edited

Loading

roccotripaldi left a comment •

edited

Loading

a8ci18n Jan 30, 2017 •

edited

Loading

MichaelArestad commented Feb 1, 2017 •

edited

Loading