-
Notifications
You must be signed in to change notification settings - Fork 13
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
Adds core support for user role widget contexts #22
base: master
Are you sure you want to change the base?
Conversation
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.
@folkhack Thanks for the pull request! Please see my comments and suggestions inline the pull request.
// | ||
|
||
// Check for logged-out user | ||
if( $settings['logged-out'] === '1' && $is_logged_in !== true ) { |
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.
@folkhack You should probably ensure that logged-out
exists before using it. A good solution is wp_parse_args()
with a set of defaults.
} | ||
|
||
$current_user = wp_get_current_user(); | ||
$is_logged_in = ( $current_user->ID !== 0 && $current_user->ID ); |
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.
Could we use is_user_logged_in()
instead?
// Build whitelist of user roles that are allowed to see widget | ||
$role_check_array = array(); | ||
|
||
foreach( $settings as $role_key => $setting_val ) { |
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 can skip all of this if the user is not logged in, correct?
asort( $roles ); | ||
|
||
// First two options are the logged-in and logged-out options | ||
$options[ 'logged-in' ] = __( 'When user is logged in (all types, global)', 'widget-context' ); |
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.
Maybe simplify these to User logged in
and User logged out
?
// Add the roles that the user can currently edit as whitelist user role options | ||
foreach( $roles as $role_key => $role_name ) { | ||
|
||
$options[ 'role-' . $role_key ] = sprintf( __( 'When user has role "%s" (%s)', 'widget-context' ), $role_name, $role_key ); |
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.
And here, something like User role "%s"
?
function get_roles() { | ||
|
||
global $wp_roles; | ||
return apply_filters( 'editable_roles', $wp_roles->roles ); |
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.
@folkhack There is get_editable_roles()
-- could we use that?
Hi there!
Long time fan, have been using your plugin for over 3 years now on 10 client sites!
Needed support for user roles within your Widget Context plugin and decided to take a Sunday afternoon and make a contribution.
Very clean code - was a joy to work within!
Thank you for your consideration, and please feel free to shoot me any feedback/critique =)
Sincerely,