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: Refresh store on Publicize module toggle via 1 call #34142

Merged
merged 7 commits into from
Nov 20, 2023
Merged

Social: Refresh store on Publicize module toggle via 1 call #34142

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
07cf749
Remove previous refresh functionality
gmjuhasz Nov 15, 2023
61facfa
Add wrapper to refresh settings in class component on Jetpack Settings
gmjuhasz Nov 15, 2023
1902f5d
Refresh on social admin page on module toggle change
gmjuhasz Nov 15, 2023
b441f37
Remove useEffect
gmjuhasz Nov 16, 2023
3e98588
Run useEffect only once on module update
gmjuhasz Nov 16, 2023
fa0dac4
Show toggles instantly on change + only refresh ones on page lifetime
gmjuhasz Nov 17, 2023
4b5f559
Update ref logic
gmjuhasz Nov 20, 2023
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
1 change: 1 addition & 0 deletions projects/js-packages/publicize-components/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export { default as PublicizePanel } from './src/components/panel';
export { default as ReviewPrompt } from './src/components/review-prompt';
export { default as PostPublishReviewPrompt } from './src/components/post-publish-review-prompt';
export { default as PostPublishOneClickSharing } from './src/components/post-publish-one-click-sharing';
export { default as RefreshJetpackSocialSettingsWrapper } from './src/components/refresh-jetpack-settings';

export { default as useSocialMediaConnections } from './src/hooks/use-social-media-connections';
export { default as useSocialMediaMessage } from './src/hooks/use-social-media-message';
Expand Down
13 changes: 0 additions & 13 deletions projects/js-packages/publicize-components/src/components/auto-conversion/toggle/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { ToggleControl } from '@automattic/jetpack-components';
import { useSelect, useDispatch } from '@wordpress/data';
import { useCallback } from '@wordpress/element';
import { useEffect } from '@wordpress/element';
import React from 'react';
import { SOCIAL_STORE_ID } from '../../../social-store';
import { SocialStoreSelectors } from '../../../types/types';
Expand All @@ -16,11 +15,6 @@ type AutoConversionToggleProps = {
* The class name to add to the toggle.
*/
toggleClass?: string;

/**
* Whether or not to refresh the settings.
*/
shouldRefresh?: boolean;
};

/**
Expand All @@ -30,16 +24,9 @@ type AutoConversionToggleProps = {
* @returns {React.ReactElement} - JSX.Element
*/
const AutoConversionToggle: React.FC< AutoConversionToggleProps > = ( {
shouldRefresh = false,
toggleClass,
children,
} ) => {
const refreshSettings = useDispatch( SOCIAL_STORE_ID ).refreshAutoConversionSettings;

useEffect( () => {
shouldRefresh && refreshSettings();
}, [ shouldRefresh, refreshSettings ] );

const { isEnabled, isUpdating } = useSelect( select => {
const store = select( SOCIAL_STORE_ID ) as SocialStoreSelectors;
return {
Expand Down
20 changes: 20 additions & 0 deletions projects/js-packages/publicize-components/src/components/refresh-jetpack-settings/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { useDispatch } from '@wordpress/data';
import { SOCIAL_STORE_ID } from '../../social-store';

/**
* HOC that refreshes all of the Jetpack Social settings in the store, to be used in class components.
*
* @param {object} props - The component props.
* @param {boolean} props.shouldRefresh - Whether or not to refresh the settings.
* @param {object} props.children - The children to render.
* @returns { object } The refreshJetpackSocialSettings function.
*/
export default function RefreshJetpackSocialSettingsWrapper( { shouldRefresh, children } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this file should be in a folder named refresh-social-settings

const refreshOptions = useDispatch( SOCIAL_STORE_ID ).refreshJetpackSocialSettings;

if ( shouldRefresh ) {
refreshOptions();
}

return children;
}
13 changes: 0 additions & 13 deletions ...s/js-packages/publicize-components/src/components/social-image-generator/toggle/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { ToggleControl } from '@automattic/jetpack-components';
import { useSelect, useDispatch } from '@wordpress/data';
import { useCallback } from '@wordpress/element';
import { useEffect } from '@wordpress/element';
import React from 'react';
import { SOCIAL_STORE_ID } from '../../../social-store';
import { SocialStoreSelectors } from '../../../types/types';
Expand All @@ -16,11 +15,6 @@ type SocialImageGeneratorToggleProps = {
* The class name to add to the toggle.
*/
toggleClass?: string;

/**
* Whether or not to refresh the settings.
*/
shouldRefresh?: boolean;
};

/**
Expand All @@ -32,14 +26,7 @@ type SocialImageGeneratorToggleProps = {
const SocialImageGeneratorToggle: React.FC< SocialImageGeneratorToggleProps > = ( {
toggleClass,
children,
shouldRefresh = false,
} ) => {
const refreshSettings = useDispatch( SOCIAL_STORE_ID ).refreshSocialImageGeneratorSettings;

useEffect( () => {
shouldRefresh && refreshSettings();
}, [ refreshSettings, shouldRefresh ] );

const { isEnabled, isUpdating } = useSelect( select => {
const store = select( SOCIAL_STORE_ID ) as SocialStoreSelectors;
return {
Expand Down
2 changes: 1 addition & 1 deletion projects/plugins/jetpack/_inc/client/sharing/features/auto-conversion-section.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { FormFieldset } from '../../components/forms';
const AutoConversionSection = () => {
return (
<FormFieldset>
<AutoConversionToggle shouldRefresh toggleClass="jp-settings-sharing__sig-toggle">
<AutoConversionToggle toggleClass="jp-settings-sharing__sig-toggle">
<div>
<div>
<Text>
Expand Down
2 changes: 1 addition & 1 deletion projects/plugins/jetpack/_inc/client/sharing/features/social-image-generator-section.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import './style.scss';
const SocialImageGeneratorSection = () => {
return (
<FormFieldset>
<SocialImageGeneratorToggle shouldRefresh toggleClass="jp-settings-sharing__sig-toggle">
<SocialImageGeneratorToggle toggleClass="jp-settings-sharing__sig-toggle">
<div>
<Text>
<strong>{ __( 'Enable Social Image Generator', 'jetpack' ) }</strong>
Expand Down
13 changes: 9 additions & 4 deletions projects/plugins/jetpack/_inc/client/sharing/publicize.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { getRedirectUrl } from '@automattic/jetpack-components';
import { RefreshJetpackSocialSettingsWrapper } from '@automattic/jetpack-publicize-components';
import { createInterpolateElement } from '@wordpress/element';
import { __, _x } from '@wordpress/i18n';
import Card from 'components/card';
Expand Down Expand Up @@ -154,10 +155,14 @@ export const Publicize = withModuleSettingsFormHelpers(
>
{ __( 'Automatically share your posts to social networks', 'jetpack' ) }
</ModuleToggle>
{ shouldShowChildElements && hasAutoConversion && <AutoConversionSection /> }
{ shouldShowChildElements && hasSocialImageGenerator && (
<SocialImageGeneratorSection />
) }
<RefreshJetpackSocialSettingsWrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we using this component here so we can encapsulate the useEffect because this is a class component?

That's probably fine, but it might be worth checking that we don't want to do something like handle it with a lifecycle method in here.

Copy link
Member

Choose a reason for hiding this comment

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

useEffect is not even needed for this and thus a lifecycle method may be a good place for it.

Copy link
Contributor Author

@gmjuhasz gmjuhasz Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, and also useDispatch. I added the useEffect to fix an issue earlier where the inner component was updated while the parent was changing, but it seems that's already fixed and it might not be needed. I think we still need the component to be able to dispatch the store function, or we need to wrap publicize.jsx into another HOC component

shouldRefresh={ ! isActive && this.props.isSavingAnyOption( 'publicize' ) }
>
{ shouldShowChildElements && hasAutoConversion && <AutoConversionSection /> }
{ shouldShowChildElements && hasSocialImageGenerator && (
<SocialImageGeneratorSection />
) }
</RefreshJetpackSocialSettingsWrapper>
</SettingsGroup>
) }

Expand Down
30 changes: 24 additions & 6 deletions projects/plugins/social/src/js/components/admin-page/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import {
} from '@automattic/jetpack-components';
import { useConnection } from '@automattic/jetpack-connection';
import { SOCIAL_STORE_ID } from '@automattic/jetpack-publicize-components';
import { useSelect } from '@wordpress/data';
import { useState, useCallback } from '@wordpress/element';
import { useSelect, useDispatch } from '@wordpress/data';
import { useState, useCallback, useEffect, useRef } from '@wordpress/element';
import React from 'react';
import AdvancedUpsellNotice from '../advanced-upsell-notice';
import AutoConversionToggle from '../auto-conversion-toggle';
Expand All @@ -28,9 +28,13 @@ const Admin = () => {
const showConnectionCard = ! isRegistered || ! isUserConnected;
const [ forceDisplayPricingPage, setForceDisplayPricingPage ] = useState( false );

const refreshJetpackSocialSettings = useDispatch( SOCIAL_STORE_ID ).refreshJetpackSocialSettings;

const onUpgradeToggle = useCallback( () => setForceDisplayPricingPage( true ), [] );
const onPricingPageDismiss = useCallback( () => setForceDisplayPricingPage( false ), [] );

const hasRefreshedSettings = useRef( false );

const {
isModuleEnabled,
showPricingPage,
Expand All @@ -56,6 +60,20 @@ const Admin = () => {
};
} );

useEffect( () => {
if (
isModuleEnabled &&
isUpdatingJetpackSettings &&
pablinos marked this conversation as resolved.
Show resolved Hide resolved
! hasRefreshedSettings.current &&
( isAutoConversionAvailable || isSocialImageGeneratorAvailable )
) {
hasRefreshedSettings.current = true;
refreshJetpackSocialSettings();
}
// We want this to run only once, when the module is enabled.
gmjuhasz marked this conversation as resolved.
Show resolved Hide resolved
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [ isModuleEnabled ] );

const moduleName = `Jetpack Social ${ pluginVersion }`;

if ( showConnectionCard ) {
Expand Down Expand Up @@ -89,11 +107,11 @@ const Admin = () => {
{ shouldShowAdvancedPlanNudge && <AdvancedUpsellNotice /> }
<InstagramNotice onUpgrade={ onUpgradeToggle } />
<SocialModuleToggle />
{ ! isUpdatingJetpackSettings && isModuleEnabled && isAutoConversionAvailable && (
<AutoConversionToggle shouldRefresh />
{ isModuleEnabled && isAutoConversionAvailable && (
<AutoConversionToggle disabled={ isUpdatingJetpackSettings } />
pablinos marked this conversation as resolved.
Show resolved Hide resolved
) }
{ ! isUpdatingJetpackSettings && isModuleEnabled && isSocialImageGeneratorAvailable && (
<SocialImageGeneratorToggle shouldRefresh />
{ isModuleEnabled && isSocialImageGeneratorAvailable && (
<SocialImageGeneratorToggle disabled={ isUpdatingJetpackSettings } />
) }
</AdminSection>
<AdminSectionHero>
Expand Down
17 changes: 4 additions & 13 deletions projects/plugins/social/src/js/components/auto-conversion-toggle/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { SOCIAL_STORE_ID } from '@automattic/jetpack-publicize-components';
import { ExternalLink } from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';
import { createInterpolateElement, useCallback } from '@wordpress/element';
import { useEffect } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import React from 'react';
import ToggleSection from '../toggle-section';
Expand All @@ -12,20 +11,12 @@ import styles from './styles.module.scss';

type AutoConversionToggleProps = {
/**
* Whether or not to refresh the settings.
* If the toggle is disabled.
*/
shouldRefresh?: boolean;
disabled?: boolean;
};

const AutoConversionToggle: React.FC< AutoConversionToggleProps > = ( {
shouldRefresh = false,
} ) => {
const refreshSettings = useDispatch( SOCIAL_STORE_ID ).refreshAutoConversionSettings;

useEffect( () => {
shouldRefresh && refreshSettings();
}, [ shouldRefresh, refreshSettings ] );

const AutoConversionToggle: React.FC< AutoConversionToggleProps > = ( { disabled } ) => {
const { isEnabled, isUpdating } = useSelect( select => {
const store = select( SOCIAL_STORE_ID ) as SocialStoreSelectors;
return {
Expand All @@ -46,7 +37,7 @@ const AutoConversionToggle: React.FC< AutoConversionToggleProps > = ( {
return (
<ToggleSection
title={ __( 'Automatically convert images for compatibility', 'jetpack-social' ) }
disabled={ isUpdating }
disabled={ isUpdating || disabled }
checked={ isEnabled }
onChange={ toggleStatus }
>
Expand Down
18 changes: 6 additions & 12 deletions projects/plugins/social/src/js/components/social-image-generator-toggle/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,16 @@ import ToggleSection from '../toggle-section';
import { SocialStoreSelectors } from '../types/types';
import styles from './styles.module.scss';

interface SocialImageGeneratorToggleProps {
type SocialImageGeneratorToggleProps = {
/**
* Whether or not to refresh the settings.
* If the toggle is disabled.
*/
shouldRefresh: boolean;
}
disabled?: boolean;
};

const SocialImageGeneratorToggle: React.FC< SocialImageGeneratorToggleProps > = ( {
shouldRefresh,
disabled,
} ) => {
const refreshSettings = useDispatch( SOCIAL_STORE_ID ).refreshSocialImageGeneratorSettings;

useEffect( () => {
shouldRefresh && refreshSettings();
}, [ shouldRefresh, refreshSettings ] );

const [ currentTemplate, setCurrentTemplate ] = useState< string | null >( null );
const { isEnabled, isUpdating, defaultTemplate } = useSelect( select => {
const store = select( SOCIAL_STORE_ID ) as SocialStoreSelectors;
Expand Down Expand Up @@ -71,7 +65,7 @@ const SocialImageGeneratorToggle: React.FC< SocialImageGeneratorToggleProps > =
return (
<ToggleSection
title={ __( 'Enable Social Image Generator', 'jetpack-social' ) }
disabled={ isUpdating }
disabled={ isUpdating || disabled }
checked={ isEnabled }
onChange={ toggleStatus }
>
Expand Down
Loading