-
Notifications
You must be signed in to change notification settings - Fork 92
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
Improve Setup Wizard #468
Improve Setup Wizard #468
Conversation
- Revise language throughout setup wizard - Stop forcing auto install - Do not show configuration nag on wizard page - Add `required` for various required form inputs - Add `password` input type where appropriate - Fix broken auto install button
98628a0
to
7ee06d3
Compare
@@ -61,7 +61,9 @@ jQuery(document).ready(function($) { | |||
Admin settings tab switching | |||
*/ | |||
var currentTab; | |||
if ( localStorageAvailable() && window.localStorage.getItem( 'Auth0WPSettingsTab' ) ) { | |||
if ( window.location.hash ) { |
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.
Use the hash if we have one so we're able to link to specific tabs in the settings panel
@@ -78,6 +80,7 @@ jQuery(document).ready(function($) { | |||
|
|||
// Set the tab showing on the form and persist the tab | |||
$( '.nav-tabs [role="tab"]' ).click( function () { | |||
window.location.hash = ''; |
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.
If we navigate to a new tab, remove the hash so we use localStorage
/* | ||
Initial setup | ||
*/ | ||
$('.js-a0-setup-input').keydown(function(e){ |
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 not submit the form if we press enter (fixes #252)
@@ -29,7 +29,7 @@ protected function showAsModal() { | |||
public function form( $instance ) { | |||
|
|||
wp_enqueue_media(); | |||
wp_enqueue_script( 'wpa0_admin', WPA0_PLUGIN_JS_URL . 'admin.js', array( 'jquery' ) ); | |||
wp_enqueue_script( 'wpa0_admin', WPA0_PLUGIN_JS_URL . 'admin.js', array( 'jquery' ), WPA0_VERSION ); |
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.
Missing a cache breaker
@@ -215,11 +215,11 @@ public function admin_enqueue() { | |||
return; | |||
} | |||
|
|||
if ( ! WP_Auth0::ready() ) { | |||
if ( ! WP_Auth0::ready() && 'wpa0-setup' !== $_REQUEST['page'] ) { |
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.
Exclude setup nag if we're on the setup page
add_action( 'admin_notices', array( $this, 'create_account_message' ) ); | ||
} | ||
|
||
if ( 'wpa0' === $wpa0_curr_page ) { | ||
if ( in_array( $wpa0_curr_page, array( 'wpa0', 'wpa0-setup' ) ) ) { |
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.
Load admin JS if we're on the setup page
@@ -240,6 +240,7 @@ public function render_allow_wordpress_login( $args = array() ) { | |||
$this->render_switch( $args['label_for'], $args['opt_name'] ); | |||
$this->render_field_description( | |||
__( 'Turn on to enable a link on wp-login.php pointing to the core login form. ', 'wp-auth0' ) . | |||
__( 'Logins and signups using the WordPress form will NOT be pushed to Auth0. ', 'wp-auth0' ) . |
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.
Clarifying this based on the setup language
<input type="text" id="admin-email" value="<?php echo $current_user->user_email; ?>" disabled /> | ||
<input type="password" id="admin-password" name="admin-password" placeholder="Password" value="" /> | ||
<input type="text" id="admin-email" value="<?php echo $current_user->user_email; ?>" disabled> | ||
<input type="password" id="admin-password" name="admin-password" placeholder="Password" value="" required> |
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.
Don't let the form submit if we don't have a password
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.
Just the password is not enough, but that's a beginning. What about email/username?
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.
It uses the current admin email address, next line above.
@@ -26,12 +44,12 @@ | |||
<div class="profile"> | |||
<img src="<?php echo WPA0_PLUGIN_URL; ?>/assets/img/initial-setup/simple-end-users-login.svg"> | |||
|
|||
<h2><?php _e( "Social Login", "wp-auth0" ); ?></h2> | |||
<h2><?php _e( "Standard", "wp-auth0" ); ?></h2> |
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.
"Social" was misleading here ... also includes Passwordless, DB, etc. Basically "non-Enterprise" and what folks usually want, so "Standard"
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'd double check the name and get a second opinion since this looks like a term you'd share among platforms and use on quickstarts.
<div class="modal-footer"> | ||
<a class="a0-button primary" href="#" id="manuallySetToken">Manual Setup (no Internet access)</a> | ||
<a class="a0-button primary submit" href="#">Automatic setup</a> | ||
<h4><?php _e( 'Standard Setup', 'wp-auth0' ) ?></h4> |
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.
New modal language and prompts to clarify what the two options will do
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.
Is this "standard" still talking about the 2 types of data-profile-type? if not, find a different word or clarify it.
</div> | ||
</div> | ||
|
||
|
||
<script type="text/javascript"> |
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.
Removed the auto-setup here (was forcing a data migration which not all sites need) and moved the rest to assets/js/admin.js
@@ -1,36 +0,0 @@ | |||
<div class="a0-wrap consent-disclaimer"> |
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.
Unused template
@jeffreylees - Added you here for a review of the documentation that I changed. All in-line, let me know if it's hard to read or if you'd rather see it in-place on a demo site. |
7ee06d3
to
c7cb02e
Compare
c7cb02e
to
f7a1000
Compare
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.
🔢
@@ -6,6 +6,17 @@ | |||
margin: 30px auto; | |||
} | |||
|
|||
.modal-open { | |||
|
|||
.modal{ |
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.
Are you going to run the code beautifier now or in a separate PR?
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 no repo-wide beautifier yet. Coming in the next release.
assets/js/admin.js
Outdated
Initial setup | ||
*/ | ||
$('.js-a0-setup-input').keydown(function(e){ | ||
if(13 === e.keyCode) { |
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.
13 would be "enter" key? add a comment for that, or use a constant.
assets/js/admin.js
Outdated
e.preventDefault(); | ||
$('#enterTokenModal').modal(); | ||
$('#connectionSelectedModal').modal('hide'); | ||
return false; |
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.
On some languages at least, this boolean that you return is for "The event has already been handled". If that's the case, why are some returning it and some others not?
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.
Legacy but I agree
<input type="text" id="admin-email" value="<?php echo $current_user->user_email; ?>" disabled /> | ||
<input type="password" id="admin-password" name="admin-password" placeholder="Password" value="" /> | ||
<input type="text" id="admin-email" value="<?php echo $current_user->user_email; ?>" disabled> | ||
<input type="password" id="admin-password" name="admin-password" placeholder="Password" value="" required> |
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.
Just the password is not enough, but that's a beginning. What about email/username?
@@ -26,12 +44,12 @@ | |||
<div class="profile"> | |||
<img src="<?php echo WPA0_PLUGIN_URL; ?>/assets/img/initial-setup/simple-end-users-login.svg"> | |||
|
|||
<h2><?php _e( "Social Login", "wp-auth0" ); ?></h2> | |||
<h2><?php _e( "Standard", "wp-auth0" ); ?></h2> |
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'd double check the name and get a second opinion since this looks like a term you'd share among platforms and use on quickstarts.
<div class="modal-footer"> | ||
<a class="a0-button primary" href="#" id="manuallySetToken">Manual Setup (no Internet access)</a> | ||
<a class="a0-button primary submit" href="#">Automatic setup</a> | ||
<h4><?php _e( 'Standard Setup', 'wp-auth0' ) ?></h4> |
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.
Is this "standard" still talking about the 2 types of data-profile-type? if not, find a different word or clarify it.
<a href="https://auth0.com/docs/api/management/v2/tokens#get-a-token-manually" target="_blank"> | ||
<?php echo __( 'token generator', 'wp-auth0' ); ?></a> | ||
<?php _e( 'token generator', 'wp-auth0' ); ?></a> |
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 won't last long (default expiration time). What do you need the token for?
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.
Setup process to create an Application and a DB connection
@@ -30,8 +30,8 @@ | |||
</div> | |||
|
|||
<div class="a0-buttons"> | |||
<a onclick="gotodashboard()" href="https://manage.auth0.com/#/clients/<?php echo WP_Auth0_Options::Instance()->get('client_id'); ?>/connections" class="a0-button primary" target="_blank"><?php _e( "GO TO DASHBOARD", "wp-auth0" ); ?></a> | |||
<a onclick="next()" href="<?php echo admin_url( 'admin.php?page=wpa0-setup&step=4' ); ?>"class="a0-button link"><?php _e( "Skip this step", "wp-auth0" ); ?></a> | |||
<a href="https://manage.auth0.com/#/clients/<?php echo WP_Auth0_Options::Instance()->get('client_id'); ?>/connections" class="a0-button primary" target="_blank"><?php _e( "GO TO DASHBOARD", "wp-auth0" ); ?></a> |
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.
fix the URL. clients -> applications
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.
👍 waiting for @jeffreylees
👍 from docs in Slack for "Standard" |
required
attribute<script>
tag JS to external file and load on Setup WizardAddresses #408