-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[Gutenberg] UBE: Pressenting Jetpack Security settings on Missing Block #13011
[Gutenberg] UBE: Pressenting Jetpack Security settings on Missing Block #13011
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
boolean unsupportedBlockEditorSwitch = !mIsJetpackSsoEnabled && "gutenberg".equals(mSite.getWebEditor()); | ||
return new GutenbergPropsBuilder( | ||
enableMentions, | ||
isUnsupportedBlockEditorEnabled, | ||
unsupportedBlockEditorSwitch, |
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.
unsupportedBlockEditorSwitch
is true
if the site is a Jetpack site. Meaning that the site can switch the SSO settings. Just Jetpack sites have this capability.
Naming is not the best, but we are trying to make its usage generic, and not refer to SSO or Jetpack directly on gutenberg
.
You can test the changes on this Pull Request by downloading the APK here. |
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.
All three scenarios are working as expected 🎉. I've left some comments on the gutenberg and gutenberg-mobile PRs.
private void setupToolbar() { | ||
setTitle(getResources().getText(R.string.jetpack_security_setting_title)); | ||
|
||
Toolbar toolbar = findViewById(org.wordpress.mobile.ReactNativeGutenbergBridge.R.id.toolbar); |
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 we need the full package name here for the String?
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.
Ah no, thanks!
Fixed.
} else if (preference == mJpUseTwoFactorPref) { | ||
mJpUseTwoFactorPref.setChecked((Boolean) newValue); | ||
mSiteSettings.enableJetpackSsoTwoFactor((Boolean) newValue); | ||
} |
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.
Not a big deal, but I think it would help readability a bit if we extracted out the Boolean
cast so that we don't have to do the cast repeatedly in the if-else block:
Boolean prefBool = (Boolean) newValue;
if (preference == mJpMonitorActivePref) {
mJpMonitorActivePref.setChecked(prefBool);
mSiteSettings.enableJetpackMonitor(prefBool);
} else if (preference == mJpMonitorEmailNotesPref) {
mJpMonitorEmailNotesPref.setChecked(prefBool);
mSiteSettings.enableJetpackMonitorEmailNotifications(prefBool);
} else if ...
If we had written this file in Kotlin, we could have also used a when
statement to make this a bit more readable:
when (preference) {
mJpMonitorActivePref -> {
mJpMonitorActivePref.setChecked(prefBool);
mSiteSettings.enableJetpackMonitor(prefBool);
}
mJpMonitorEmailNotesPref -> {
mJpMonitorEmailNotesPref.setChecked(prefBool);
mSiteSettings.enableJetpackMonitorEmailNotifications(prefBool);
}
}
...
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.
You are right. Thanks! Fixed.
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.
Reviewed the code, and this looks good. Left a couple of small comments, but nothing I consider blocking. Nice job! 👍
Thanks a lot for extra effort! |
Many thanks!!! |
…-to-enable-jetpack-sso
…kEditorSwitch to canEnableUnsupportedBlockEditor
Gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#2610
This PR implements changes from wordpress-mobile/gutenberg-mobile#2610 to present the Jetpack Security settings modally over Gutenberg, and refresh the editor capabilities to enable the Unsupported Block Editor.
NOTE: The action button text for UBE Enabled has been updated to
Edit using web editor
.To test:
Jetpack connected sites (Self-Hosted and Atomic)
Jetpack - UBE Disabled
.Open Jetpack security settings
.Allow WordPress.com login
and press Done.Jetpack - UBE Enabled
from the image.Edit using web editor
.Self-Hosted sites.
Self-Hosted - Disabled
.WPCom Simple sites.
On a WPCom Simple site:
Open a post with an unsupported block in it.
Press the (?) button on the missing block.
WPCom Simple - Enabled
.Press the action button
Edit using web editor
.I have considered adding unit tests where possible.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to
RELEASE-NOTES.txt
if necessary.