Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Admin Page: Make settings updates have better notice messages #10972

Merged
merged 5 commits into from
Dec 17, 2018
Merged

Admin Page: Make settings updates have better notice messages #10972

merged 5 commits into from
Dec 17, 2018

Conversation

oskosk
Copy link
Contributor

@oskosk oskosk commented Dec 13, 2018 •
edited
Loading

Addresses #10640 partially. The scope of the messaging updates needs the structure introduced in this PR but has a wider reach than just the accelerator message.

Changes proposed in this Pull Request:

  • Adds custom notice message when updating settings via the Site accelerator toggle. d35b2a7
  • Refactors the Redux action creator updateSettings to handle the custom message in a tidier way. With a api response mapping function. 7464fad
  • Also the action creator is updated to accept a messages parameter with an object holding the notice messages as strings. Refactors the High Order component connect-module-options to better handle the regeneratePostByEmailAddress() request and also to accept the messages parameter for the updateOptions method. 4a19c58

Testing instructions

for designers

  1. Check this branch, or launch a JN site with this branch
  2. Connect the site.
  3. Jumpstart the site
  4. On the Wirting settings tab, enable and disable site accelerator from the main toggle.
  5. Check the notice messages.

Testing instructions for developers

As this PR refactors stuff affecting other settings please check the above, but also

  1. Regenerating a post by email address and expecting the same messages as currently
  2. Updating any other setting, and seeing the generic message for settings update.

Proposed changelog entry for your changes:

  • Added better message for settings notices.

Screeshots

activation

@oskosk oskosk added [Status] In Progress Admin Page React-powered dashboard under the Jetpack menu labels Dec 13, 2018
@oskosk oskosk requested a review from a team December 13, 2018 18:09
@jetpackbot
Copy link
Collaborator

jetpackbot commented Dec 13, 2018 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 10, 2019.
Scheduled code freeze: January 3, 2019

Generated by 🚫 dangerJS against 3be4e83

@oskosk oskosk mentioned this pull request Dec 13, 2018
Open
@oskosk oskosk added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Dec 13, 2018
@jeherve jeherve requested a review from a team December 13, 2018 21:27
oskosk
oskosk commented Dec 13, 2018
View reviewed changes
_inc/client/state/settings/actions.js
return ( dispatch ) => {
let messages = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment for the reviewer. Basically this block was moved into mapUpdateSettingsResponseFromApi and the messages have been delegated to the component dispatching the action to one of the actions creators mapped into its props.

oskosk
oskosk commented Dec 13, 2018
View reviewed changes
_inc/client/writing/speed-up-site.jsx
if ( false === ! newPhotonStatus && 'active' !== photonStatus ) {
newPhotonStatus = false;

this.props.updateOptions( {
Copy link
Contributor Author

@oskosk oskosk Dec 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment for the reviewer: There were two calls to this.props.updateOptions() being done here. Now, let's collect the settings, and only then call the action creator once.

@oskosk oskosk requested a review from jeherve December 13, 2018 22:34
@oskosk oskosk added the [Status] Needs Design Review Design has been added. Needs a review! label Dec 13, 2018
@oskosk
Copy link
Contributor Author

oskosk commented Dec 13, 2018 •
edited
Loading

You can see in the gif, that the only customization provided by this PR is the site accelerator notice for when updating the general toggle.

keoshi
keoshi previously approved these changes Dec 17, 2018
View reviewed changes
Copy link
Contributor

@keoshi keoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the note I've added about capitalization this works great in my testing!

_inc/client/writing/speed-up-site.jsx Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Dec 17, 2018
jeherve
jeherve approved these changes Dec 17, 2018
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well for me, and gives us the opportunity to change more and more settings in the future with updateOptions. 👍

@oskosk oskosk added [Status] Design Review Complete and removed [Status] Needs Design Review Design has been added. Needs a review! labels Dec 17, 2018
@oskosk oskosk merged commit 3c25b0d into master Dec 17, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Dec 17, 2018
@oskosk oskosk deleted the update/make-settings-update-have-better-notice-messages branch December 17, 2018 14:43
@jeffgolenski
Copy link
Member

Thanks Osk. the functionality works great.

@joanrho joanrho mentioned this pull request Dec 18, 2018
Closed
jeherve added a commit that referenced this pull request Dec 19, 2018
@jeherve
changelog: add #10972
ad14251
jeherve added a commit that referenced this pull request Jan 3, 2019
@jeherve
changelog: add #10972
9b16fc6
jeherve added a commit that referenced this pull request Jan 3, 2019
@jeherve
Jetpack 6.9: Changelog and testing list (#10722)
8d91603 
* Add first version of the Changelog and testing list for 6.9

* Changelog: add #10710

* changelog: add #10538

* changelog: add #10741

* changelog: add #10749

* changelog: add #10664

* changelog: add #10224

* changelog: add #10788

* Changelog: add #10560

* Chanegelog: add #10812

* changelog: add #10556

* Changelog: add #10668

* Changelog: add #10846

* Changelog: add #10947

* Changelog: add #10962

* Changelog: add #10956

* Changelog: add #10940

* Changelog: add #10934

* Changelog: add #10912

* changelog: add #10866

* changelog: add #10924

* Changelog: add #10936

* Changelog: add #10833

* changelog: add #10867

* Changelog: add #10960

* Changelog: add #10888

* changelog: add #10840

* changelog: add #10972

* Changelog: add #10979

* changelog: add #10909

* Changelog: add #10958

* Changelog: add #10981

* Changelog: add #10564

* Changelog: add #10809

* Changelog: add #10982

* Changelog: add #10706

* Changelog: add #10978

* Changelog: add #10132

* Changelog: add #11022

* Changelog: add #11024

* Changelog: add #10875

* Changelog: add #11030

* Changelog: add #11053

* Changelog: add #10880

* Changelog: add #9359

* Changelog: add #11037

* Update block list

* Changelog: add #11060

* Changelog: add #10755

* changelog: add #11000

* Changelog: add #10786

* Changelog: add #10945

* Changelog: add #10597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keoshi keoshi keoshi left review comments

@jeherve jeherve jeherve approved these changes

Assignees

@oskosk oskosk

Labels
Admin Page React-powered dashboard under the Jetpack menu [Status] Design Review Complete
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@oskosk @jetpackbot @jeffgolenski @keoshi @jeherve @matticbot