-
Notifications
You must be signed in to change notification settings - Fork 16
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
Hours of Operation Widget #22
base: develop
Are you sure you want to change the base?
Conversation
…between open and close times.
/** | ||
* Hours of Operation Widget | ||
*/ | ||
.wphoow-widget .form div { |
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.
@EvanHerman Let's use the wpcw prefix for consistency
@@ -2,7 +2,7 @@ | |||
/** | |||
* Plugin Name: Contact Widgets | |||
* Description: Beautifully display social media and contact information on your website with these simple widgets. | |||
* Version: 1.3.3 | |||
* Version: 1.3.4 |
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.
@EvanHerman Don't increment the version number until we decide to release the plugin.
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.
@jbardo I think the version was incremented because we tagged the previous release when we bumped the tested up to version to 4.7.
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.
@EvanHerman That's weird, it shouldn't show off as a change then. Okay moving on!
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.
@jonathanbardo I think the issue was that the version was bumped in the master
branch, but the develop
branch versions didn't get bumped (that was the only difference between the branches) - so I just pulled what we had on master
and started working off that so everything was in sync moving forward.
|
||
$field['name'] = str_replace( 'value', strtolower( $day ), $field['name'] ); | ||
|
||
$field['disabled'] = ( $hours['not_open'] ) ? true : 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.
@EvanHerman No need for the parenthesis here.
*/ | ||
protected function render_day_input( $field, $day, array $hours ) { | ||
|
||
$field['name'] = str_replace( 'value', strtolower( $day ), $field['name'] ); |
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.
@EvanHerman I think you can align the equal sign of those 2 assignation since they deal with fields elements.
|
||
$disabled_field = $field['disabled'] ? ' disabled="disabled"' : ''; | ||
|
||
echo '<select name="' . esc_attr( $field['name'] . '[open]' ) . '"' . $disabled_field . '>'; |
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.
@EvanHerman Seems like there is a lot of echo
in this block. It might look nicer if we close the php tag and use html directly.
@EvanHerman I feel we might be too restrictive. What about we propose a free text alternative on every field (that way if they close for lunch they can manually add the instruction)? @fjarrett Any thoughts? |
@jonathanbardo I see what you mean, but we need to use micro-formats which will be difficult to display semantically in a machine-readable way without a day/open/close data pattern to work with. https://schema.org/openingHours |
@fjarrett I agree, I wouldn't make it default for sure but I want to be as inclusive as we can. The use case we solved won't work for most European countries so I'm thinking we should at least have the possibility of free text. Also, I think we should add an extra textarea at the end for further details (for example: closed during holidays and 1 month in summer etc). |
@jonathanbardo Yeah that's a fantastic idea! |
…s all days, tweaks per @jbardos comments.
@jonathanbardo How do you propose we add the text field for free text? Should it be visible at all times, and just override the Also, why wouldn't this scale for most European countries? |
@EvanHerman I know a lot of European countries where they close during lunch time or are totally closed in August for example. With the current implementation it would be impossible for them to express that. As for the free text I would say we should add a toggle to add more details and hide it by default. |
@jonathanbardo Right, right. I forgot about the lunch breaks in other countries - good catch, didn't cross my mind. 👍 I will get working on the micro formats, free text and additional info fields! |
'sanitizer' => function( $value ) { return current_user_can( 'unfiltered_html' ) ? $value : wp_kses_post( stripslashes( $value ) ); }, | ||
'escaper' => function( $value ) { return nl2br( apply_filters( 'widget_text', $value ) ); }, | ||
'form_callback' => 'render_form_textarea', | ||
'description' => __( 'Enter additional information about your store.', 'contact-widgets' ), |
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.
@EvanHerman I think Enter additional information about your business.
might be more universal.
@EvanHerman Travis seems to be failing https://travis-ci.org/godaddy/wp-contact-widgets/jobs/183766078. |
@jonathanbardo The acceptance tests should be passing now. I'll still need to write some acceptance tests for the new Hours of Operation widget when all of the features are built out. |
$hidden_text_field = ( ! $hours['custom_text_checkbox'] ) ? ' style="display:none;"' : ''; | ||
$custom_text_field = '<input type="text" name="' . $field['name'] . '[custom_text]" id="' . $field['name'] . '[custom_text]" class="widefat custom_text_field" value="' . $hours['custom_text'] . '"' . $hidden_text_field . '/>'; | ||
$closed_checkbox = '<input name="' . $field['name'] . '[not_open]" id="' . $field['name'] . '[not_open]" class="js_wpcw_closed_checkbox" type="checkbox" value="1" ' . $this->checked( $hours['not_open'], true ) . '><label for="' . $field['name'] . '[not_open]" class="js_wpcw_closed_checkbox"><small>' . esc_html__( 'Closed', 'contact-widgets' ) . '</small></label>'; | ||
|
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.
@jonathanbardo This section is a bit verbose. Not sure there is a better way to handle it without introducing additional code, which I didn't want to introduce additional bloat.
$field['name'] = str_replace( 'value', strtolower( $day ), $field['name'] ); | ||
|
||
$disabled_field = $field['disabled'] ? ' disabled="disabled"' : ''; | ||
$hidden_field = $hours['custom_text_checkbox'] ? ' style="display:none;"' : ''; |
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.
@jonathanbardo Looking back on this it may be best to utilize classes for hidden. I'm not a fan of inline styles. Thoughts?
*/ | ||
protected function get_half_hour_time_array() { | ||
|
||
$half_hour_steps = range( 0, 47 * 1800, 1800 ); |
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.
@jonathanbardo @fjarrett What do you think about introducing a filter here to allow users to set increments to whole hours, 15 mins, 10 mins etc.? Do you think that would be beneficial?
$hours = $store_hours['not_open'] ? __( 'Closed', 'contact-widgets' ) : $store_hours['open'] . apply_filters( 'wpcw_hours_seperator', ' - ' ) . $store_hours['closed']; | ||
$class = $store_hours['not_open'] ? 'closed' : 'open'; | ||
|
||
if ( $store_hours['custom_text_checkbox'] ) { |
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's worth noting here that when a user has a 'custom text' specified the micro formats are not added to the hours of operation item.
I can't quite figure out why the acceptance tests are failing either. I can see why in the log, but it looks like it should be passing. |
@EvanHerman Sometimes Selenium is acting weird. When only 1 builds fails I normally try to restart that particular build. I will find some time to look into why it's acting so weird in travis. |
|
||
if ( time_blocks.length ) { | ||
|
||
for ( i = 0; i < time_blocks.length; i++ ) { |
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.
@EvanHerman I'm about to change the get_schedule()
method output slightly to use a key/value pair for open/close, so this loop will need to change to:
$.each( time_blocks, function( open, close ) {
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.
@fjarrett 👍 No problem. I can adjust after the change.
$close_select = $parent.find( '.time-block-close' ); | ||
|
||
$close_select.children().show(); | ||
$close_select.find( 'option[value="' + time + '"]' ).prevAll().andSelf().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.
@EvanHerman .andSelf()
is deprecated and replaced by .addBack()
|
||
global $_wp_admin_css_colors; | ||
|
||
$active_scheme = $_wp_admin_css_colors[ get_user_option( 'admin_color' ) ]; |
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.
@EvanHerman Bro!!! Nice one. I didn't know this was possible. 💯
@@ -518,12 +610,33 @@ public function enqueue_scripts() { | |||
$suffix = SCRIPT_DEBUG ? '' : '.min'; | |||
|
|||
wp_enqueue_style( 'font-awesome', '//maxcdn.bootstrapcdn.com/font-awesome/4.5.0/css/font-awesome.min.css', [], '4.5.0' ); | |||
wp_enqueue_style( 'wpcw-admin', \Contact_Widgets::$assets_url . "css/admin{$suffix}.css", [ 'font-awesome' ], Plugin::$version ); | |||
wp_enqueue_script( 'wpcw-admin', \Contact_Widgets::$assets_url . "js/admin{$suffix}.js", [ 'jquery' ], Plugin::$version, true ); | |||
wp_enqueue_style( 'jquery-timepicker', Plugin::$assets_url . "css/jquery.timepicker{$suffix}.css", [ 'font-awesome' ], '1.11.9' ); |
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.
@EvanHerman Since the timepicker is used exclusively by the hours widget, it shouldn't go in the base widget. Instead, we should utilize inheritance inside class-hours.php
. Additionally, we should do the same for font-awesome
and move it to class-social.php
.
Another thing to try would be to add an arg to the enqueue_scripts()
method that would allow localize script data to be passed to from every inherited method back to the parent base.
The code below is untested, but it's an example of what I mean:
/* class-hours.php */
public function enqueue_scripts( $localize_script_data = [] ) {
$localize_script_data = [
'time_format' => get_option( 'time_format' ),
];
parent::enqueue_scripts( $localize_script_data );
// Stuff here the hours widget needs to enqueue, like jquery-timepicker
}
/* class-base-widget.php */
public function enqueue_scripts( $localize_script_data = [] ) {
// Stuff here every widget needs to enqueue, like wpcw-admin
static $data = []; // Defaults here
$data = array_merge_recursive( $data, $localize_script_data );
if ( $data ) {
wp_localize_script( 'wpcw-admin', 'wpcw_admin', $data );
}
}
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.
Well, since every widget shares the same JS file that expects the wpcw_admin
global to always contain certain values, maybe the localize script data thing isn't necessary after all.
But the general idea of using inheritance for enqueuing other scripts and styles like jquery-timepicker
and font-awesome
still applies.
@fjarrett I believe in the latest commit I incorporated what you were referencing. I also went ahead and bumped font awesome up to 4.7.0, since 4.5.0 is somewhat outdated. I added the version number as a class property so we can easily bump it in future. I know Font awesome 5 is in the works. |
Creating an hours of operation widget.