-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Post By Email: add front-end settings for Simple sites #90819
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~213 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
client/my-sites/site-settings/publishing-tools/post-by-email.tsx
Outdated
Show resolved
Hide resolved
client/my-sites/site-settings/publishing-tools/post-by-email.tsx
Outdated
Show resolved
Hide resolved
client/my-sites/site-settings/publishing-tools/post-by-email.tsx
Outdated
Show resolved
Hide resolved
df66af6
to
c503407
Compare
client/my-sites/site-settings/publishing-tools/post-by-email.tsx
Outdated
Show resolved
Hide resolved
client/my-sites/site-settings/publishing-tools/post-by-email.tsx
Outdated
Show resolved
Hide resolved
client/my-sites/site-settings/publishing-tools/post-by-email.tsx
Outdated
Show resolved
Hide resolved
client/my-sites/site-settings/publishing-tools/post-by-email.tsx
Outdated
Show resolved
Hide resolved
Thanks again for looking this over @nightnei. I've addressed your feedback and also gone through and cleaned things up a bit. A big part of that was making sure the variables were named it ways that made it clear which path they were a part of (e.g. This means the variable names are more verbose than they were before. I think the names I've chosen make the code easier to read, but please let me know if you disagree of if I've overcompensated! |
client/my-sites/site-settings/publishing-tools/post-by-email.tsx
Outdated
Show resolved
Hide resolved
Regarding test instructions - sometimes a reviewer is not in the context of the changes, honestly even for me it's totally not clear what is
Here is typo, right? UPDATE:
Test steps for Simple website
Test steps for Atomic website
Offtopic: Newbie question - WoA is WordPress on Atomic, right? Here I would specify that "now we will be testing Atomic website, so necessary to create another website and make it Atomic", since initially I was confused with short sentense and so quick and dramatic "jump" from Simple to Atomic, w/o explanation that now we will be testing another thing. |
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.
In general everything look cool 👍 And works will (both UI and backend). I haven't only tested the last two steps in your tet instructions, but I tested PbE in Default(Calypso) View and it works well, as previously.
Left a few comments and will be waiting for rebasing with trunk
to make a final review.
client/my-sites/site-settings/publishing-tools/post-by-email.tsx
Outdated
Show resolved
Hide resolved
client/my-sites/site-settings/publishing-tools/post-by-email.tsx
Outdated
Show resolved
Hide resolved
client/my-sites/site-settings/publishing-tools/post-by-email.tsx
Outdated
Show resolved
Hide resolved
client/my-sites/site-settings/publishing-tools/post-by-email.tsx
Outdated
Show resolved
Hide resolved
disabled={ | ||
isFormPending || | ||
jetpackRegeneratingPostByEmail || | ||
isSimpleSitePendingRegenerate || | ||
! isActive || | ||
!! moduleUnavailable |
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 have different condition for disabled state here and here (https://github.com/Automattic/wp-calypso/pull/90819/files#diff-1a2d2a711ac2389148e7764b72177d565e3e6a88fea709f208906a031a5a64c5R142)
- Let's make it the same (I thing we can add
isFormPending
to the "Copy" button) - Let's create variable for both cases to avoid duplication
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.
This one was actually a bit of an interesting rabbit hole once I started looking at it more closely.
There are five disabled
props in play:
- Jetpack toggle
- Simple site toggle
- Copy button,
- Regenerate button
is-disabled
classname on theFormLabel
.- Across those five items, we actually had four different conditions. Technically only four ever appear at once, but still... that's way too many combinations of conditions!
To clean this up, I've made it so the three items that apply to both flows are the same using a new, single variable to check the relevant conditions. I've also made it so isFormPending
is included in all conditions, as that's not flow-specific and checks to see if any settings are being requested for the current site.
One other interesting discovery... for the new Simple site toggle, we disable the toggle itself if the setting is being toggled or if the email is being regenerated. I think that's something I initially included when taking inspiration from earlier versions of your Post by Voice PR.
We don't actually have the same behavior for Jetpack, because:
- We don't have a Jetpack version of
isSimpleSitePendingToggle
(theisUpdatingJetpackSettings
selector might suffice, but I'll need to look at it a bit more) - While we do have
jetPackRegeneratingPostByEmail
, I don't see it actually doing anything. It doesn't apply a disabled state, or change the button text the way the code currently asks it to. It doesn't even log out astrue
when the regeneration button is pressed. I'm not sure if it just isn't implemented correctly, or if there's some other problem.
When I'm back on tuesday, I'll prioritize looking into jetPackRegeneratingPostByEmail
and isUpdatingJetpackSettings
to see if they can serve the same roles as our Simple counterparts, or if they should just be excluded entirely for now.
But either way, the disabled logic should be a little simpler now... and might become more so if we decide to go without jetPackRegeneratingPostByEmail
.
3cb3546
to
a14c278
Compare
Thank you @nightnei! I appreciate the feedback on the testing instructions, and have made some changes there 🙂
Yep! Sorry for throwing the extra acronym around! -- Thanks for all of the other feedback, I believe I've addressed everything, I'll just need to circle back to the |
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.
Looking good and tests well! 👍
I left a couple of small remarks.
{ renderPostByVoice && this.renderPostByVoiceModule() } | ||
{ renderPostByEmail && this.renderPostByEmailModule() } | ||
{ this.renderPostByEmailModule() } |
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 think we should put Post by Email above Post by Voice since it's used much more frequently.
const moduleUnavailable = siteInDevMode && moduleUnavailableInDevMode; | ||
const { data: simpleSitePostByEmailSettings } = useGetPostByEmail( selectedSiteId ); |
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.
const moduleUnavailable = siteInDevMode && moduleUnavailableInDevMode; | |
const { data: simpleSitePostByEmailSettings } = useGetPostByEmail( selectedSiteId ); | |
const moduleUnavailable = siteInDevMode && moduleUnavailableInDevMode; | |
const { data: simpleSitePostByEmailSettings } = useGetPostByEmail( selectedSiteId ); |
Nit, but a newline here would be nice to delineate between Jetpack and Simple stuff.
<ToggleControl | ||
checked={ !! simpleSitePostByEmailSettings?.isEnabled } | ||
disabled={ isFormPending || isSimpleSitePendingToggle || isSimpleSitePendingRegenerate } | ||
label={ translate( 'Post by Email' ) } |
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.
label={ translate( 'Post by Email' ) } | |
label={ translate( 'Publish posts by sending an email' ) } |
It seems logical to reuse the Jetpack control label here.
const jetpackRegeneratingPostByEmail = useSelector( ( state ) => | ||
isRegeneratingJetpackPostByEmail( state, selectedSiteId ) | ||
); |
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.
This is outside the scope of this PR, but in my testing, this selector doesn't seem to work properly (it never returns true
). The same behavior can be observed in production.
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 you think it's worth removing from Post by Email for now, so it's not misleading/confusing for devs reading this code in the future? Or leave it to quietly do nothing in case/until it's fixed in the future?
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.
Scratch that question... we were really only trying to use it for one thing that I decided to remove anyway, so that selector is no longer called at all. If it's ever fixed we can bring it back to have parity with Simple sites, but for now we can leave it out, I think.
const isDisabledForBothFlows = | ||
isFormPending || | ||
jetpackRegeneratingPostByEmail || | ||
isSimpleSitePendingRegenerate || | ||
! isActive || | ||
!! moduleUnavailable; | ||
|
||
return ( | ||
<> | ||
<FormFieldset> | ||
<SupportInfo | ||
text={ translate( | ||
'Allows you to publish new posts by sending an email to a special address.' | ||
) } | ||
link={ | ||
siteIsAtomic | ||
? localizeUrl( 'https://wordpress.com/support/post-by-email/' ) | ||
: 'https://jetpack.com/support/post-by-email/' | ||
} | ||
privacyLink={ ! siteIsAtomic } | ||
/> | ||
{ siteIsJetpack ? ( | ||
<JetpackModuleToggle | ||
siteId={ selectedSiteId } | ||
moduleSlug="post-by-email" | ||
label={ translate( 'Publish posts by sending an email' ) } | ||
disabled={ isFormPending || moduleUnavailable } | ||
/> | ||
) : ( | ||
<ToggleControl | ||
checked={ !! simpleSitePostByEmailSettings?.isEnabled } | ||
disabled={ isFormPending || isSimpleSitePendingToggle || isSimpleSitePendingRegenerate } |
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.
const isDisabledForBothFlows = | |
isFormPending || | |
jetpackRegeneratingPostByEmail || | |
isSimpleSitePendingRegenerate || | |
! isActive || | |
!! moduleUnavailable; | |
return ( | |
<> | |
<FormFieldset> | |
<SupportInfo | |
text={ translate( | |
'Allows you to publish new posts by sending an email to a special address.' | |
) } | |
link={ | |
siteIsAtomic | |
? localizeUrl( 'https://wordpress.com/support/post-by-email/' ) | |
: 'https://jetpack.com/support/post-by-email/' | |
} | |
privacyLink={ ! siteIsAtomic } | |
/> | |
{ siteIsJetpack ? ( | |
<JetpackModuleToggle | |
siteId={ selectedSiteId } | |
moduleSlug="post-by-email" | |
label={ translate( 'Publish posts by sending an email' ) } | |
disabled={ isFormPending || moduleUnavailable } | |
/> | |
) : ( | |
<ToggleControl | |
checked={ !! simpleSitePostByEmailSettings?.isEnabled } | |
disabled={ isFormPending || isSimpleSitePendingToggle || isSimpleSitePendingRegenerate } | |
const isJetpackDisabled = isFormPending || moduleUnavailable; | |
const isSimpleSiteDisabled = | |
isFormPending || isSimpleSitePendingToggle || isSimpleSitePendingRegenerate; | |
const isDisabledControls = ! isActive || isJetpackDisabled || isSimpleSiteDisabled; | |
return ( | |
<> | |
<FormFieldset> | |
<SupportInfo | |
text={ translate( | |
'Allows you to publish new posts by sending an email to a special address.' | |
) } | |
link={ | |
siteIsAtomic | |
? localizeUrl( 'https://wordpress.com/support/post-by-email/' ) | |
: 'https://jetpack.com/support/post-by-email/' | |
} | |
privacyLink={ ! siteIsAtomic } | |
/> | |
{ siteIsJetpack ? ( | |
<JetpackModuleToggle | |
siteId={ selectedSiteId } | |
moduleSlug="post-by-email" | |
label={ translate( 'Publish posts by sending an email' ) } | |
disabled={ isJetpackDisabled } | |
/> | |
) : ( | |
<ToggleControl | |
checked={ !! simpleSitePostByEmailSettings?.isEnabled } | |
disabled={ isSimpleSiteDisabled } |
It'd be nice to assign the disabled
values for the respective controls to separate variables.
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.
Oh yes, that's much cleaner, thank you!
|
||
export const getPostByEmailPath = ( siteId: number | null ) => `/sites/${ siteId }/post-by-email`; | ||
|
||
export const getCachePostByEmailKey = ( siteId: number | null ) => [ |
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.
export const getCachePostByEmailKey = ( siteId: number | null ) => [ | |
export const getPostByEmailKeyQueryKey = ( siteId: number | null ) => [ |
I overlooked this in #90489, but I think we should mention "query key" rather than "cache key".
import PressThis from '../press-this'; | ||
import { PostByEmailSetting } from './post-by-email'; | ||
import { PostByVoiceSetting } from './post-by-voice'; |
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.
import PressThis from '../press-this'; | |
import { PostByEmailSetting } from './post-by-email'; | |
import { PostByVoiceSetting } from './post-by-voice'; | |
import PressThis from 'calypso/my-sites/site-settings/press-this'; | |
import { PostByEmailSetting } from 'calypso/my-sites/site-settings/publishing-tools/post-by-email'; | |
import { PostByVoiceSetting } from 'calypso/my-sites/site-settings/publishing-tools/post-by-voice'; |
Nit, but let's be consistent with absolute import paths.
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.
Shoot! I made a point of fixing that, because it was mentioned on a previous PR. I must have lost it during a rebase 🤦
Co-authored-by: Fredrik Rombach Ekelund <[email protected]>
Co-authored-by: Volodymyr Makukha <[email protected]>
650f148
to
ac575ce
Compare
value={ email } | ||
/> | ||
<Button onClick={ handleRegenerate } disabled={ isDisabledControls }> | ||
Regenerate address |
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.
There was previously (including on trunk
) logic to change this button content to Regenerating...
while a new address was being generated for Jetpack sites. I've removed that in favor of consistent button text for the reasons below.
It isn't working for Jetpack (including on trunk) as Fredrik and I noted above, and when I applied it to Simple sites (where it does work) I really didn't like the way it changed the size of the button. The visual jump was king of jarring. Could be fixed with CSS, but it honestly didn't feel like it was worth it.
On Simple, we're also now temporarily disabling the button while the new address is generated, so there's visual feedback in that form, and if/when the isRegeneratingJetpackPostByEmail
is working in the future, we can apply the temp disable effect there, too.
tl;dr: from a user perspective, removing this text change doesn't impact Jetpack users at all, and we have something else in place for Simple that I think works better anyway.
Current feedback has been addressed, and I've added one new inline comment regarding a small change when regenerating the email address. Based on past feedback I think we should be good, or very very close! |
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 made some light tweaks in 09e5011 and then some slightly larger refactorings of the PublishingTools
component in 6cf91ac and 2c7e20c. All of the Post by Email-related logic is consolidated in PostByEmailSetting
now.
I've tested this with a Simple site, an Atomic site, and an external Jetpack site.
This should be good to land 👍
export default connect( | ||
( state ) => { | ||
const selectedSiteId = getSelectedSiteId( state ); | ||
const siteIsJetpack = isJetpackSite( state, selectedSiteId ); | ||
const isAtomic = isSiteAutomatedTransfer( state, selectedSiteId ); | ||
const regeneratingPostByEmail = isRegeneratingJetpackPostByEmail( state, selectedSiteId ); | ||
const siteInDevMode = isJetpackSiteInDevelopmentMode( state, selectedSiteId ); | ||
const moduleUnavailableInDevMode = isJetpackModuleUnavailableInDevelopmentMode( | ||
state, | ||
selectedSiteId, | ||
'post-by-email' | ||
); | ||
|
||
return { | ||
siteIsJetpack, | ||
isAtomic, | ||
selectedSiteId, | ||
regeneratingPostByEmail, | ||
postByEmailAddressModuleActive: !! isJetpackModuleActive( | ||
state, | ||
selectedSiteId, | ||
'post-by-email' | ||
), | ||
moduleUnavailable: siteInDevMode && moduleUnavailableInDevMode, | ||
}; | ||
}, | ||
{ | ||
regeneratePostByEmail, | ||
} | ||
)( localize( PublishingTools ) ); |
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.
My goal here wasn't to remove the connect
call. However, I discovered that several props were redundant (a lot of logic has been moved to PostByEmailSetting
) and that SiteSettingsFormWriting
can pass the other props we need.
export const PostByEmailSetting = ( { emailAddress }: PostByEmailSettingProps ) => { | ||
const selectedSiteId = useSelector( getSelectedSiteId ) || 0; | ||
const siteIsJetpack = useSelector( ( state ) => isJetpackSite( state, selectedSiteId ) ); | ||
const jetpackRegeneratingPostByEmail = siteIsJetpack && emailAddress === 'regenerate'; |
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 isRegeneratingJetpackPostByEmail
selector doesn't work, but we can deduce the same thing by determining if emailAddress
has the value regenerate
. "One weird trick" 🙃
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.
This almost feels sneaky, and I love it 😆
link={ | ||
! siteIsJetpack | ||
? localizeUrl( 'https://wordpress.com/support/post-by-email/' ) | ||
: 'https://jetpack.com/support/post-by-email/' | ||
} | ||
privacyLink={ siteIsJetpack } |
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 used to look at siteIsAtomic
here and the logic was flipped (Simple sites linked to jetpack.com and Atomic sites to wordpress.com).
The flipped logic was obviously wrong, but I also don't see a reason to look at siteIsAtomic
instead of siteIsJetpack
.
componentDidUpdate() { | ||
const { | ||
fields, | ||
moduleUnavailable, | ||
postByEmailAddressModuleActive, | ||
regeneratingPostByEmail, | ||
selectedSiteId, | ||
} = this.props; | ||
|
||
if ( ! moduleUnavailable ) { | ||
return; | ||
} | ||
|
||
if ( | ||
postByEmailAddressModuleActive && | ||
regeneratingPostByEmail === null && | ||
! fields.post_by_email_address | ||
) { | ||
this.props.regeneratePostByEmail( selectedSiteId ); | ||
} | ||
} | ||
|
||
onRegenerateButtonClick = () => { | ||
this.props.regeneratePostByEmail( this.props.selectedSiteId ); | ||
}; | ||
|
||
isFormPending() { | ||
const { isRequestingSettings, isSavingSettings } = this.props; | ||
|
||
return isRequestingSettings || isSavingSettings; | ||
} |
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.
Initially, I thought we could remove this altogether, but I realized that the componentDidUpdate
logic is still required. What it does is regenerate the email address immediately after the Jetpack Post by Email module is enabled for the first time. Without it, the feature can be enabled, but there's no email address to send to.
I moved this functionality to PostByEmailSetting
in 2c7e20c
@@ -8,15 +8,15 @@ import type { | |||
|
|||
export const getPostByVoicePath = ( siteId: number | null ) => `/sites/${ siteId }/post-by-voice`; | |||
|
|||
export const getCachePostByVoiceKey = ( siteId: number | null ) => [ | |||
export const getPostByVoiceQueryKey = ( siteId: number | null ) => [ |
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.
For consistency, I took the opportunity to rename this.
Thanks for sharing those updates @fredrikekelund. Testing well on my end, too! |
Related to https://github.com/Automattic/dotcom-forge/issues/7210
Proposed Changes
Why are these changes being made?
Notes
JetpackModuleToggle
as the setting did previously, or just aToggleControl
from @wordpress/components. I'd like to look into whether or not we can use theToggleControl
in both cases and just modify its handlers for Atomic/Jetpack vs Simple sites.Testing Instructions
wp-admin/admin.php?page=jetpack#/writing
, making sure the Calypso settings still properly update the Jetpack settings (we didn't make changes here, but the code was moved and refactored so we're making sure nothing got broken in the process)