-
Notifications
You must be signed in to change notification settings - Fork 800
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
Publicize: make the default social media message blank #21477
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
57ea2f6
to
537c7ec
Compare
This is a bit tricky, as the end result depends on the platform. On Facebook for example (one of our most popular Publicize options), if you leave that field empty we will use the excerpt. I'll cc @pablinos, who's been working on quite a few improvements to this interface in the past few weeks. |
Yes, we've been considering this a little. I've been starting to think that we should leave the textbox blank because, as @jeherve mentions, the default message changes based on the network being shared to. It would also simplify things as we have code in place to work out if the message has been edited or not. I'd prefer that we shorten the message to something like, "Write a message for your audience here" and rely on some other help to let people know the message that we'll use. We can also rely on on the social previews feature, although that might need updating to make sure that it's a true preview. |
d7c9c7f
to
5d9d1ef
Compare
This is ready for review! |
it seems the testing instructions are not updated according to the latest changes. Is it right? |
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.
3d6ae05
to
6bca5cd
Compare
Hi! This just needs another approval after rebase. |
Accepted. :-) |
if ( isTweetStorm() ) { | ||
if ( postTitle ) { | ||
return postTitle.substr( 0, getShareMessageMaxLength() ) + DEFAULT_TWEETSTORM_MESSAGE; | ||
} |
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.
Do we still need this if we're relying on the back end to create the default message? Without the hasEditedShareMessage
logic, this text will look like it has been added by the user, and will be used when sharing the message. That might not be an issue as long as it's in sync with whatever would be produced server side.
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.
That's interesting! I didn't know the server side generated the message. I'm not too familiar with this, do you think I should remove this check? Looking at this code, it does seem like this JS code is redundant.
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.
Yes, that's what I'm thinking, although I notice the default there doesn't include the post title. We probably need to run some tests, but it would make sense to have all this logic in one place.
If we need to, we could add the title of the post to the default custom message.
We could merge this and deal with the tweetstorm logic as a separate change.
Great news! One last step: head over to your WordPress.com diff, D68773-code, and commit it. Thank you! |
r234186-wpcom |
…ng-formatting * master: docker: Add script to create "fake" translations for testing i18n (#21407) Add webpack-config package (#21482) Update dependency composer/semver to v3.2.6 (#21608) Update dependency eslint-plugin-import to v2.25.2 (#21607) Update JS unit testing packages (#21605) Admin page: Fix pricing/currency displayed on the upgrade page (#21594) Update dependency @rollup/plugin-node-resolve to v13.0.6 (#21599) Publicize: Add new publicize icon toggle component (#20957) Search: hide search menu for atomic sites (#21565) Allow /wp/v2/sites/1234/batch endpoint to process widget updates (#21549) tooling: Add `.mjs` as a JavaScript extension (#21589) Jetpack: PHPCS src/class-tracking (#21583) Publicize: make the default social media message blank (#21477)
* master: docker: Add script to create "fake" translations for testing i18n (#21407) Add webpack-config package (#21482) Update dependency composer/semver to v3.2.6 (#21608) Update dependency eslint-plugin-import to v2.25.2 (#21607) Update JS unit testing packages (#21605) Admin page: Fix pricing/currency displayed on the upgrade page (#21594) Update dependency @rollup/plugin-node-resolve to v13.0.6 (#21599) Publicize: Add new publicize icon toggle component (#20957) Search: hide search menu for atomic sites (#21565) Allow /wp/v2/sites/1234/batch endpoint to process widget updates (#21549) tooling: Add `.mjs` as a JavaScript extension (#21589) Jetpack: PHPCS src/class-tracking (#21583) Publicize: make the default social media message blank (#21477)
* master: (40 commits) docker: Add script to create "fake" translations for testing i18n (#21407) Add webpack-config package (#21482) Update dependency composer/semver to v3.2.6 (#21608) Update dependency eslint-plugin-import to v2.25.2 (#21607) Update JS unit testing packages (#21605) Admin page: Fix pricing/currency displayed on the upgrade page (#21594) Update dependency @rollup/plugin-node-resolve to v13.0.6 (#21599) Publicize: Add new publicize icon toggle component (#20957) Search: hide search menu for atomic sites (#21565) Allow /wp/v2/sites/1234/batch endpoint to process widget updates (#21549) tooling: Add `.mjs` as a JavaScript extension (#21589) Jetpack: PHPCS src/class-tracking (#21583) Publicize: make the default social media message blank (#21477) Photon: do not serve Wikipedia images from CDN. (#21572) Publicize: update endpoint (#21510) [Plugin] Backup: Update initial backup screen (#21559) RNA Connect Screen: Remove unused files (#21570) RNA Connection: Add ConnectScreenRequiredPlan Component (#21521) Update PHPUnit coverage configs (#21557) cli: Fix skeleton phpunit config (#21555) ...
Currently, the Publicize message field has the following placeholder
While in reality we use the post title.
I realize this solution is hard to make perfect in every language, but I think it's good enough. If you think it's not, changing the placeholder or leaving the default message empty might be more fool-proof solutions.Fixes Automattic/wp-calypso#57144 and #21530
Changes proposed in this Pull Request:
This PR proposes making it blank.
Does this pull request change what data or activity we track or use?
No.
Testing instructions: