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

Social: Fix share modal trigger presence #39135

Merged
merged 3 commits into from
Aug 30, 2024
Merged

Social: Fix share modal trigger presence #39135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
efd0f15
Only show modal trigger if there are shares for the post
gmjuhasz Aug 29, 2024
2aae9ac
changelog
gmjuhasz Aug 29, 2024
42eb3c5
Use only socialStore
gmjuhasz Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions projects/js-packages/publicize-components/changelog/fix-social-share-modal-trigger-presence
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: fixed

Fixed a bug on when to show the share log modal trigger
7 changes: 6 additions & 1 deletion projects/js-packages/publicize-components/src/components/share-status/modal-trigger.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,13 @@ type ModalTriggerProps = ButtonProps & {
export const ModalTrigger = forwardRef(
( { withWrapper = false, ...props }: ModalTriggerProps, ref: unknown ) => {
const { openShareStatusModal } = useDispatch( socialStore );

const featureFlags = useSelect( select => select( socialStore ).featureFlags(), [] );
const shareStatus = useSelect( select => select( socialStore ).getPostShareStatus(), [] );
Copy link
Member

Choose a reason for hiding this comment

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

We need to check for feature flag here

const shareStatus = useSelect(
	select => {
		return featureFlags.useShareStatus
			? select( socialStore ).getPostShareStatus()
			: { shares: [] };
	},
	[ featureFlags.useShareStatus ]
);

// If the post is not shared anywhere, thus there are no shares, we don't need to show the trigger.
if ( ! featureFlags.useShareStatus || ! shareStatus.shares.length ) {
	return null;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we do that here, or inside the getPostShareStatus resolver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we could do something like

		const featureFlags = registry.select( socialStore ).featureFlags();

		if ( ! featureFlags.useShareStatus ) {
			dispatch( fetchPostShareStatus( postId, false ) );
			return;
		}

into getPostShareStatus in projects/js-packages/publicize-components/src/social-store/resolvers.js so it's checked wherever its called

Copy link
Member

Choose a reason for hiding this comment

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

I had thought the same but it's better to keep the resolver clean.

Copy link
Member

Choose a reason for hiding this comment

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

Or may be it's fine to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's true, but then we have to do that logic you mentioned everywhere 🤔 I'm not sure which one is better, I thought in the resolver it's less places to do

Copy link
Member

Choose a reason for hiding this comment

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

OK, let us go with the resolver approach to do an early return based on feature flag


// If the post is not shared anywhere, thus there is no share status or no shares, we don't need to show the trigger.
if ( ! shareStatus || ! shareStatus.shares || shareStatus.shares.length === 0 ) {
return null;
}

if ( ! featureFlags.useShareStatus ) {
return null;
Expand Down
Loading