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

Publicize: Sync WordPress.com and Jetpack UI #10043

Merged
merged 15 commits into from
Oct 24, 2018
Merged

Publicize: Sync WordPress.com and Jetpack UI #10043

merged 15 commits into from
Oct 24, 2018

Conversation

mdawaffe
Copy link
Member

@mdawaffe mdawaffe commented Aug 22, 2018 •
edited
Loading

Changes proposed in this Pull Request:

  • New (and already deprecated) filter for customizing Publicize Settings URL. Do not use :) It will go away.
  • New (and already deprecated) filter for customizing Publicize Settings Page. Also do not use :) Same.
  • Code formatting tweaks
  • Translation fixes (mostly /* translators: */ comments).
  • Improve default message handling: Show as placeholder if textarea is empty
  • Improve custom message handling: On page load, if there is a "non-trivial" custom message (one that is not empty and does not match the default message), open the UI so that the author remembers. (Use case: when a custom message is explicitly set to the default message, then the post title changes.)

Testing instructions:

Does it Work?
  1. Add at least two Publicize connections on a test site.
  2. Create a post in the Classic (non-Gutenberg) editor.
  3. Set it to publicize to one connection, but not the others.
  4. Save as Draft.
  5. See that the Publicize settings are correct after saving the draft (set to publicize to one connection, but not the others).
  6. Publish the post.
  7. See the publicize results on the one connection (e.g., the tweet).
  8. In the post editor, see all Publicize checkboxes disabled (this is the same as the old behavior).
Publicize Message UI
  1. In the Classic (non-Gutenberg) editor, create a new post.
  2. Set the title to "Hello World".
  3. Set the contents to "Yay".
  4. Save as Draft.
  5. See the Publicize settings are still closed on the new page load after saving the draft.
  6. Edit the Publicize message. Erase all contents in the message.
  7. See the message box's placeholder matches the post's title.
  8. Save as draft.
  9. See the Publicize message UI is still closed on the new page load after saving the draft.
  10. Edit the Publicize message. Set the Publicize message to "Something else".
  11. Save as draft.
  12. See the Publicize message UI is now open on the new page load after saving the draft.
  13. Edit the Publicize settings. Set the Publicize message to "Hello World".
  14. Save as draft.
  15. See the Publicize message UI is again closed on the new page load after saving the
    draft.
  16. Set the title to "New Title"
  17. Save as Draft.
  18. See the Publicize message UI is again open on the new page load after saving the
    draft.

Proposed changelog entry for your changes:

Updated Jetpack's and WordPress.com's Publicize user interface to match.

@mdawaffe mdawaffe requested a review from a team as a code owner August 22, 2018 22:44
@mdawaffe mdawaffe added [Feature] Publicize Now Jetpack Social, auto-sharing [Status] Needs Review This PR is ready for review. DO NOT MERGE don't merge it! labels Aug 22, 2018
@mdawaffe mdawaffe self-assigned this Aug 22, 2018
@jetpackbot
Copy link
Collaborator

jetpackbot commented Aug 22, 2018 •
edited
Loading

Thank you for the great PR description!

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

Scheduled Jetpack release: November 6, 2018.
Scheduled code freeze: October 30, 2018

Generated by 🚫 dangerJS

@brbrr brbrr added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Aug 23, 2018
@gravityrail
Copy link
Contributor

Did you see this other PR where @Tug brought across the publicize connect button from Calypso? It's a prototype of an unrelated feature so unlikely to be merged before this release.

Might be of interest if we decide to move the wp-admin UI into the Jetpack Settings area.

@Tug
Copy link
Contributor

Tug commented Aug 24, 2018 •
edited
Loading

Just a note that it's still a prototype and it's super limited at the moment:

  • it's hacky
  • it doesn't work if publicize is disabled
  • it needs a page refresh if user enables publicize from the settings
  • etc.

The approach taken here looks way more solid 👍

@mdawaffe mdawaffe mentioned this pull request Aug 24, 2018
Closed
@dereksmart dereksmart added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Sep 18, 2018
@mdawaffe @ockham
Publicize: Sync with WordPress.com
be0a8cf
@mdawaffe @ockham
Publicize: Fix translation bugs
78c4dfe
@mdawaffe @ockham
Publicize: Sync WordPress.com's custom Settings URL.
5f504ce 
This will go away in a future version of Jetpack.
@mdawaffe @ockham
Publicize: Sync WordPress.com's custom Settings Page.
cedbf56 
This will go away in a future version of Jetpack.
@mdawaffe @ockham
Publicize: Show Custom Message UI if Set
ddeb302 
If a Custom Message has been set (on an earlier page load), open the UI on
page load so that the author doesn't forget.

Makes it easier to see that the Custom Message may need updating if, for
example, the post title changes.
@mdawaffe @ockham
Publicize: Keep Track of Active Service Doneness
62caf50 
In addition to looking at the `$all_done` flag, keep track of whether all
active services are done.
@mdawaffe @ockham
Publicize: Clarify API
f38f8e5 
Part of unification of WP.com and Jetpack Publicize code.
@ockham ockham force-pushed the sync/publicize-ui branch from 4f02ff7 to f38f8e5 Compare October 19, 2018 14:50
@mdawaffe mdawaffe requested a review from a team October 19, 2018 14:50
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D19658-code. (newly created revision)

@ockham
Copy link
Contributor

ockham commented Oct 19, 2018

Rebased.

@kraftbj
Copy link
Contributor

kraftbj commented Oct 19, 2018

We'll need to do some tender loving care for the WP.com. Since Fusion isn't applying the diff cleanly, I'm going to mark this as okay for the wpcom test.

@kraftbj
Copy link
Contributor

kraftbj commented Oct 23, 2018

I'm not sure about getting the bot to ack the other diff, but we can manually force the wpcom test to pass. I did so now, but I believe we'll have to redo it if there are future commits before it can merge.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D19827-code. (newly created revision)

@mdawaffe
Publicize: Make jetpack_override_publicize_settings_page filter eas…
7af6bf7 
…ier to use.

It's deprecated, but let's still make it simpler.

Props @ockham.
@mdawaffe mdawaffe added [Status] Needs Review This PR is ready for review. and removed DO NOT MERGE don't merge it! [Status] In Progress labels Oct 24, 2018
@mdawaffe mdawaffe added this to the 6.7 milestone Oct 24, 2018
ockham
ockham previously approved these changes Oct 24, 2018
View reviewed changes
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Went through testing instructions, works like a charm! :shipit:

@kraftbj
Copy link
Contributor

kraftbj commented Oct 24, 2018 •
edited
Loading

Seeing a warning on JN: Notice: Trying to get property of non-object in /srv/public/wp-content/plugins/jetpack-dev/modules/publicize/ui.php on line 735

Edit: Otherwise, it works, just need to prevent the warning.

kraftbj
kraftbj previously requested changes Oct 24, 2018
View reviewed changes
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Seeing a warning on JN: Notice: Trying to get property of non-object in /srv/public/wp-content/plugins/jetpack-dev/modules/publicize/ui.php on line 735

@mdawaffe
Publicize: Simplify meta lookup logic
ea6e6b6 
Connections are arrays in Jetpack and objects in WordPress.com.

* When looking for their corresponding services' names, just look at the
  service itself instead of the connection's copy.
* Ensure we're accessing their data via helper methods (like
  `->get_connection_meta()`).
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D19827-code. (updated diff)

@mdawaffe
Copy link
Member Author

Seeing a warning on JN: Notice: Trying to get property of non-object in /srv/public/wp-content/plugins/jetpack-dev/modules/publicize/ui.php on line 735

@kraftbj, Thanks! This should be fixed as of ea6e6b6. Could you take another look?

oskosk
oskosk approved these changes Oct 24, 2018
View reviewed changes
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

Tested perfectly with provided instructions! :shipit:

@oskosk oskosk dismissed kraftbj’s stale review October 24, 2018 17:47

Dismissing this review as I saw no warning when testing and feedback was addressed

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 24, 2018
@mdawaffe mdawaffe merged commit 5d8f264 into master Oct 24, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 24, 2018
@mdawaffe mdawaffe deleted the sync/publicize-ui branch October 24, 2018 18:02
mdawaffe added a commit that referenced this pull request Oct 24, 2018
@mdawaffe
Publicize: Decouple Connection List from UI
b17be1f 
Based on #9955 by @c-shultz.

"Rebased" on top of #10043

Moves Connection List Data "getter" out of `Publicize_UI` and in to
`Publicize_Base`.
@mdawaffe mdawaffe mentioned this pull request Oct 24, 2018
Merged
@kraftbj
Copy link
Contributor

kraftbj commented Oct 24, 2018

Sync'd to wp.com via r182397-wpcom and r182406-wpcom

mdawaffe added a commit that referenced this pull request Oct 30, 2018
@mdawaffe
Decouple Publicize Connection Lists from UI Take 2 (#10396)
20da538 
Publicize: Decouple Connection List from UI

Based on #9955 by @c-shultz. See #9039. Updated to work on top of #10043.

* Move Connection List Data "getter" out of `Publicize_UI` and in to `Publicize_Base` to clean up logic v. presentation boundary in `Publicize_UI`.
* In the wp-admin/ Publicize UI, separate the comma separated list of connections by commas :)
  The JS does this. So should the PHP.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oskosk oskosk oskosk approved these changes

@kraftbj kraftbj kraftbj left review comments

@ockham ockham ockham left review comments

Assignees

@mdawaffe mdawaffe

Labels
[Feature] Publicize Now Jetpack Social, auto-sharing Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
6.7
Development

Successfully merging this pull request may close these issues.
10 participants
@mdawaffe @jetpackbot @gravityrail @Tug @matticbot @ockham @kraftbj @oskosk @brbrr @dereksmart