-
Notifications
You must be signed in to change notification settings - Fork 153
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
Enable wordpress.com Oath2 support #58
Enable wordpress.com Oath2 support #58
Conversation
Looks good from a reading of the code, but I haven't tested |
…some language updates
var hideItemsRequiringAuth = function() { | ||
oauthconnectionestablished = document.getElementsByClassName( 'oauth-connection-established' ); | ||
if ( 0 === oauthconnectionestablished.length ) { | ||
$( '.hide-until-authed' ).hide(); |
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.
Can we cache this selector?
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.
yep, done
$( externalConnectionTypeField ).on( 'change', function( event ) { | ||
var slug = externalConnectionTypeField.value; | ||
|
||
$( '.auth-credentials' ).hide(); |
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.
Can we cache this selector?
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.
yep
$( changeCredentials ).on( 'click', function() { | ||
|
||
// Show the credentials fields. | ||
$( '.oauth-authentication-details-wrapper' ).show(); |
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.
Can we cache this selector?
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.
done
|
||
// Handle click to the wpdotcom begin-authorization button. | ||
$( beginAuthorize ).on( 'click', function( event ) { | ||
var $titleEl = $( document.getElementById( 'title' ) ), |
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 is already a selector for this
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.
used that now
* | ||
* @param string $error_message The error message to log. | ||
*/ | ||
public static function log_authentication_error( $error_message ) { |
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 like this
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.
👍
@@ -312,15 +312,15 @@ public function push( $post_id, $args = array() ) { | |||
|
|||
if ( function_exists( 'vip_safe_wp_remote_get' ) ) { | |||
$response = vip_safe_wp_remote_get( | |||
$types_path, $this->auth_handler->format_get_args( array( |
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.
Why remove this?
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.
hmm, i think that is a mistake, will restore
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.
Fixed in f24fe0b
@adamsilverstein here's my latest round of QA:
|
…ault, return false to disable.
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.
Things look alright, aside from merge conflicts, what else is needed at your end? Or is this pretty much done?
@tomjn Thanks... Yes, we have completed our internal review as well, and I will go ahead and get this merged. My next planned step is to commit the plugin with a client theme for a complete review; I will reference this pull request on the commit. |
# Conflicts: # assets/css/sass/admin-external-connection.scss # assets/js/src/admin-external-connection.js # includes/classes/ExternalConnections/WordPressExternalConnection.php
POC for review.