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 media validation deadlock #38933

Merged
merged 9 commits into from
Aug 28, 2024
Merged

Social: Fix media validation deadlock #38933

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0628513
Show SharePostForm if there is attached media
gmjuhasz Aug 16, 2024
a9724e1
changelog
gmjuhasz Aug 16, 2024
ae7c3d0
Merge branch 'trunk' into fix/social-media-validation-deadlock
gmjuhasz Aug 21, 2024
0e3167c
Show form if there is any validation error
gmjuhasz Aug 21, 2024
a806222
Use only !isConvertible
gmjuhasz Aug 21, 2024
9f3c520
Use validationErrors
gmjuhasz Aug 22, 2024
3f27ace
Merge branch 'trunk' into fix/social-media-validation-deadlock
gmjuhasz Aug 26, 2024
0eb7571
Add a small comment to explain why we do that
gmjuhasz Aug 26, 2024
3e475dd
Merge branch 'trunk' into fix/social-media-validation-deadlock
gmjuhasz Aug 28, 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-media-validation-deadlock
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 deadlock with media validation and media picker
18 changes: 14 additions & 4 deletions projects/js-packages/publicize-components/src/components/form/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ import { Disabled, PanelRow } from '@wordpress/components';
import { useSelect } from '@wordpress/data';
import { Fragment } from '@wordpress/element';
import { usePublicizeConfig } from '../../..';
import useAttachedMedia from '../../hooks/use-attached-media';
import useFeaturedImage from '../../hooks/use-featured-image';
import useMediaDetails from '../../hooks/use-media-details';
import useMediaRestrictions from '../../hooks/use-media-restrictions';
import useSocialMediaConnections from '../../hooks/use-social-media-connections';
import { store as socialStore } from '../../social-store';
import { ThemedConnectionsModal as ManageConnectionsModal } from '../manage-connections-modal';
Expand All @@ -25,8 +29,16 @@ import { SharePostForm } from './share-post-form';
* @return {object} - Publicize form component.
*/
export default function PublicizeForm() {
const { hasConnections, hasEnabledConnections } = useSocialMediaConnections();
const { hasConnections, hasEnabledConnections, connections } = useSocialMediaConnections();
const { isPublicizeEnabled, isPublicizeDisabledBySitePlan } = usePublicizeConfig();
const { attachedMedia } = useAttachedMedia();
const featuredImageId = useFeaturedImage();

const mediaId = attachedMedia[ 0 ]?.id || featuredImageId;
Copy link
Member

Choose a reason for hiding this comment

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

Why should featured image be a part of this logic?

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 that if the featured image is invalid, you still get the deadlock in the editor on the jetpack tab, until you remove the featured image. If we use it here, they can also use the attached media to upload a working image without having to change the featured image. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. Thank you for the explanation.

const { isConvertible } = useMediaRestrictions( connections, useMediaDetails( mediaId )[ 0 ] );

const showSharePostForm =
isPublicizeEnabled && ( hasEnabledConnections || attachedMedia.length > 0 || ! isConvertible );
Copy link
Member

Choose a reason for hiding this comment

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

isConvertible is false if there is no media attached. Thus ! isConvertible becomes true.

That means, the SharePostForm is visible by default, which is not the case on trunk.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we need the full logic as before

Copy link
Member

Choose a reason for hiding this comment

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

May be we don't need to use isConvertible here?

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 think we do because otherwise we would show the modal even if the media is invalid but convertible. For example the featured image is too big, but can be converted, in that case I think we do not need it 🤔 This logic starts to get fiddly

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this logic is going to be difficult to untangle in future.

May be we can add explanatory comments to that code.


const { useAdminUiV1, featureFlags } = useSelect( select => {
const store = select( socialStore );
Expand Down Expand Up @@ -57,9 +69,7 @@ export default function PublicizeForm() {

{ ! isPublicizeDisabledBySitePlan && (
<Fragment>
{ isPublicizeEnabled && hasEnabledConnections && (
<SharePostForm analyticsData={ { location: 'editor' } } />
) }
{ showSharePostForm && <SharePostForm analyticsData={ { location: 'editor' } } /> }
</Fragment>
) }
</Wrapper>
Expand Down
Loading