-
-
Notifications
You must be signed in to change notification settings - Fork 83
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
Swapped out series post widget image location for image size + alignment options #1734
Swapped out series post widget image location for image size + alignment options #1734
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.
✅ A review job has been created and sent to the PullRequest network.
Check the status or cancel PullRequest code review here.
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.
@@ -28,6 +28,7 @@ function widget( $args, $instance ) { | |||
$instance['title_link'] = get_term_link( (int) $instance['series'], 'series' ); | |||
$term = get_term( $instance['series'], 'series' ); | |||
$title = apply_filters( 'widget_title', $term->name, $instance, $this->id_base ); | |||
$thumb = isset( $instance['thumbnail_display'] ) ? $instance['thumbnail_display'] : 'small'; |
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.
was it intended to change the default from medium -> small? Also, is it possible that thumbnail_display could have an unexpected size value and does this need to be handled?
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.
The idea was to copy the functionality from one of our other widgets, so the small
default is ok.
It will be a selected from a dropdown, so I doubt there would be an unexpected size value. @benlk Do you know if that's ever happened with the Largo Recent Posts widget that this was copied from?
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 don't know if it has ever happened.
The template that this widget renders only has cases for small
, medium
, and large
, so an invalid choice or the none
option would result in no thumbnail displaying:
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 code review was cancelled. See Details
@@ -28,6 +28,7 @@ function widget( $args, $instance ) { | |||
$instance['title_link'] = get_term_link( (int) $instance['series'], 'series' ); | |||
$term = get_term( $instance['series'], 'series' ); | |||
$title = apply_filters( 'widget_title', $term->name, $instance, $this->id_base ); | |||
$thumb = isset( $instance['thumbnail_display'] ) ? $instance['thumbnail_display'] : 'small'; |
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 don't know if it has ever happened.
The template that this widget renders only has cases for small
, medium
, and large
, so an invalid choice or the none
option would result in no thumbnail displaying:
@benlk I removed this CSS in 15c20e3 so the image doesn't have a max-width constriction anymore, since the Recent Posts widget images don't have one:
|
Changes
This pull request makes the following changes:
Thumbnail position on first post
option in the Largo Series Posts widget and replaces it with Thumbnail image size and image alignment options, just like in the Largo Recent Posts widget.Before:
After:
Why
For #1727
Testing/Questions
Features that this PR affects:
Questions that need to be answered before merging:
Steps to test this PR: