-
Notifications
You must be signed in to change notification settings - Fork 799
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
Social: Add Social Share Status modal for published posts #39051
Conversation
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. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! |
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
projects/js-packages/publicize-components/src/hooks/use-post-meta/index.js
Outdated
Show resolved
Hide resolved
projects/js-packages/publicize-components/src/components/share-status-modal/share-info.tsx
Outdated
Show resolved
Hide resolved
shareStatus.shares = shareStatus.shares.filter( share => { | ||
const hasSuccessfulShareLater = shareStatus.shares.some( otherShare => { | ||
return ( | ||
otherShare.timestamp > share.timestamp && | ||
'success' === otherShare.status && | ||
otherShare.external_id === share.external_id && | ||
share.external_id // We added external_id later to the object | ||
); | ||
} ); | ||
|
||
if ( 'failure' === share.status ) { | ||
return ! hasSuccessfulShareLater; | ||
} | ||
|
||
return true; | ||
} ); |
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.
If we want to always show everything this part needs to be changed, or removed.
If we want to hide Retry for those that was already retried, we can update the status here to be retried
or something like that.
p1724761254288879/1724758671.904879-slack-C02JJ910CNL
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.
Yeah, let's show the retry button as is and not hide them.
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.
OK, let us then leave it as is for now and may be come back later to improve if needed.
I think it does make sense to treat it as activity log.
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.
I will give it a test tomorrow, @gmjuhasz.
We can reinstate the try again button and not hide it conditionally.
I am sure you must have, but would be good to try resharing a post multiple times/have many connections, and ensure the modal is scrollable.
projects/js-packages/publicize-components/src/components/post-publish-share-status/index.tsx
Outdated
Show resolved
Hide resolved
projects/js-packages/publicize-components/src/social-store/types.ts
Outdated
Show resolved
Hide resolved
projects/js-packages/publicize-components/src/utils/share-status.ts
Outdated
Show resolved
Hide resolved
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.
It works well. I only have that concern about modal height.
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.
We can fix the mobile view in another task.
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.
The happy case works fine.
A few things to address:
- When a post wasn't shared to any social connections, the review share status button still shows up. To test this publish a post, but don't share it to any connections. Go to the wp-admin's post list screen and visit the edit post view of the same post
- Let's rename review sharing status to 'View sharing history'. Can do this later as well.
- Test Mastodon
- Test LinkedIn - the name of the connection didn't show up in the modal
- Should we introduce some error handling to prevent editor crashing? Is a try/catch block possible? I am talking about it in the context of this fix - 3c5e987.
* @return {import('react').ReactNode} - React element | ||
*/ | ||
export function ShareInfo( { share } ) { | ||
const { service, external_name, profile_picture, timestamp, status, message } = share; |
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.
What happens external_name
and profile_picture
is undefined?
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.
If external_name is undefined it won't be shown as with the case of linkedIN in your example, I'm not sure why linkedIn has no external name, but we might have to send down another data to fix that if for LinkedIn that's undefined all the time. If there is no profile picture, you get the basic connection icon as with LinkedIn in your case
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.
Should we fallback to the network logo if there is no profile picture?
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.
ConnectionIcon already does that, you can see it on Sid's screenshot for Linkedin
These can be handled in a different task as this one is more about the modal.
The place where it erred, we cannot use try/catch |
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.
We can make the improvements in a follow-up. Sounds good.
Fixes https://github.com/Automattic/jetpack-reach/issues/532
Proposed changes:
Other information:
Jetpack product discussion
https://github.com/Automattic/jetpack-reach/issues/532
Does this pull request change what data or activity we track or use?
Testing instructions:
await wp.data.dispatch( 'jetpack-social-plugin').invalidateResolution( 'getPostShareStatus', [<POST_ID>] );
which invalidates the store and retrives the shares again