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

Removed more largo_featured_widget and restructured largo_about_widget #1469

Closed
wants to merge 6 commits into from

Conversation

mikeschinkel
Copy link

Also restructured largo_about_widget to ensure no undeclared errors, added comments, created private initialization function for $instance for DRY, added esc_attr() is several places and made the code a little more readable.

@mikeschinkel mikeschinkel changed the title Removed more largo_featured_widget and restructure Removed more largo_featured_widget and restructured largo_about_widget Aug 19, 2017
Copy link
Collaborator

@benlk benlk left a comment

Choose a reason for hiding this comment

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

Updates to the Largo About widget look good!

Comments for #1467 need some revision. The logic changes to largo_time_diff should not be in this PR.

? human_time_diff( $time, current_time( 'timestamp' ) )
: date( 'F j, Y', $time );

return sprintf( __( '<span class="time-ago">%s ago</span>', 'largo' ), $time );
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the case of $time_difference being > 86400 seconds (1 day), this results in the output string being:

<span class="time-ago">August 23, 2017 ago</span>

Can you remove this change from this PR, please?

$message = sprintf(
__( '%sYou have not set a description for your site.%s Add a site description by visiting %sthe Largo Theme Options page%s.', 'largo' ),
'<strong>','</strong>',
"<a href=\"{$options_url}\" title=\"{$link_title}\">", '</a>'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use tabs for this indentation.

/**
* @see https://github.com/INN/largo/issues/1467
*/
// if ( ! dynamic_sidebar( 'footer-2' ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commenting out this specific line prevents the footer-2 sidebar from displaying, since calling dynamic_sidebar( 'footer-2' ) is what causes that sidebar to be displayed. It may be best to comment out only the lines within the conditional, as you did in partials/footer-widget-3col-equal.php in this PR.

/**
* @see https://github.com/INN/largo/issues/1467
*/
// if ( ! dynamic_sidebar( 'footer-2' ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commenting out this specific line prevents the footer-2 sidebar from displaying, since calling dynamic_sidebar( 'footer-2' ) is what causes that sidebar to be displayed. It may be best to comment out only the lines within the conditional, as you did in partials/footer-widget-3col-equal.php in this PR.

@mikeschinkel
Copy link
Author

@benlk Sorry, the contributor day had lots of time pressure, so I didn't test like I should have. I will try to get to this over the weekend, if I can get my client obligations done before the end of the weekend.

@benlk
Copy link
Collaborator

benlk commented Sep 12, 2018

Hi Mike!
Do you mind if I copy this PR to make the requested changes? You'd still receive credit in the changelog and release notes.

@mikeschinkel
Copy link
Author

mikeschinkel commented Sep 12, 2018

@benlk Of course, go for it.

benlk added a commit that referenced this pull request Nov 8, 2018
Copies part of #1469 into the 0.5-dev branch.

Co-authored-by: @mikeschinkel <mike@newclarity.net>
benlk added a commit that referenced this pull request Nov 8, 2018
benlk added a commit that referenced this pull request Nov 8, 2018
@benlk
Copy link
Collaborator

benlk commented Dec 6, 2018

Finished in #1563; thanks for the work!

@benlk benlk closed this Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants