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.

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

Change Basic settings field display; better admin UX #433

Merged
merged 4 commits into from
Apr 17, 2018

Conversation

joshcanhelp
Copy link
Contributor

  • Refactored Basic settings with new render functions, new descriptions
  • Fixed PHP < 5.4 array style for advanced render functions
  • Persist tab showing on settings page
  • Universal show/hide JS for switch fields
  • Constant for JWKS cache transient name

@joshcanhelp joshcanhelp added this to the v3-Next milestone Apr 16, 2018
@joshcanhelp joshcanhelp self-assigned this Apr 16, 2018
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

Left some comments

// Show/hide field for specific switches
$('[data-expand!=""]').each( function() {
var $thisSwitch = $( this );
var $showFieldRow = $( '#' + $thisSwitch.attr( 'data-expand' ) ).closest( 'tr' );
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Length is always there for a jQuery selector. Next line checks that

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the "data-expand" and then "closest TR".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having data-expand is what gets you into the each here. No data-expand, no each

function localStorageAvailable() {
try {
var x = '__storage_test__';
window.localStorage.setItem(x, x);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this try/catch the only way to check for availability? And, can the removeItem fail after the first one succeeded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but it's the best way:

https://developer.mozilla.org/en-US/docs/Web/API/Web_Storage_API/Using_the_Web_Storage_API#Feature-detecting_localStorage

As for removeItem, that would be a weird case, not implementing the webStorage interface. But localStorage persist forever and we don't want to store something we're never going to use.

Copy link
Contributor

Choose a reason for hiding this comment

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

is this stored under some domain key or can it collide with existing "x" stored keys? In that case, you might want to use some key like "a0_local_storage_assertion"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's per-domain:

https://stackoverflow.com/q/4201239/728480

But you have a good point about that. WP sites could load a million other JS libraries.

public function render_client_secret_b64_encoded( $args = array() ) {
$this->render_switch( $args['label_for'], $args['opt_name'] );
$this->render_field_description(
__( 'Enable this if your Client Secret is base64 enabled. ', 'wp-auth0' ) .
Copy link
Contributor

Choose a reason for hiding this comment

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

base64 encoded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why did I say "enabled" here? 😆

Fixed!

$this->render_field_description(
sprintf( '<a href="https://%s/.well-known/jwks.json" target="_blank">%s</a>',
$domain,
__( 'View your JWKS here', 'wp-auth0' )
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this section show the JWKs contents or what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Links to it, yes.

@joshcanhelp joshcanhelp force-pushed the change-settings-fields-display-pt-2 branch from 23de224 to c5b6c64 Compare April 16, 2018 19:36
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

@joshcanhelp joshcanhelp merged commit de3d25e into dev Apr 17, 2018
@joshcanhelp joshcanhelp deleted the change-settings-fields-display-pt-2 branch April 20, 2018 16:45
@joshcanhelp joshcanhelp mentioned this pull request Jun 5, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants