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

Swapped out series post widget image location for image size + alignment options #1734

Merged

Conversation

joshdarby
Copy link
Collaborator

@joshdarby joshdarby commented Jul 3, 2019

Changes

This pull request makes the following changes:

  • Swaps out the 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:
Screen Shot 2019-07-03 at 12 17 54 PM

After:
Screen Shot 2019-07-03 at 12 18 04 PM

Why

For #1727

Testing/Questions

Features that this PR affects:

  • Largo Series Posts widget

Questions that need to be answered before merging:

  • Any other options we want to add/remove from this widget?

Steps to test this PR:

  1. Add Largo Series Posts widget to a sidebar
  2. Select series with posts in it
  3. Select thumbnail image and alignment
  4. Load post on frontend and make sure the widget respects selected thumbnail image and alignment options.

@joshdarby joshdarby requested a review from benlk July 3, 2019 16:20
@joshdarby joshdarby self-assigned this Jul 3, 2019
Copy link

@pullrequest pullrequest bot left a 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.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

lgtm


Reviewed with ❤️ by PullRequest

@@ -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';
Copy link

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?

Copy link
Collaborator Author

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?

Copy link
Collaborator

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:

https://github.com/INN/largo/blob/67b4dedb252220515243756e96ed2033dd7f0d38/partials/widget-content.php#L9-L30

@joshdarby joshdarby added this to the 0.6.4 milestone Jul 8, 2019
@joshdarby joshdarby added priority: high Either blocks work on a priority-normal task or a solution here informs other work. type: feature request Estimate: < 4 Hours labels Jul 8, 2019
Copy link

@pullrequest pullrequest bot left a 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';
Copy link
Collaborator

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:

https://github.com/INN/largo/blob/67b4dedb252220515243756e96ed2033dd7f0d38/partials/widget-content.php#L9-L30

inc/widgets/largo-series-posts.php Show resolved Hide resolved
@benlk
Copy link
Collaborator

benlk commented Jul 9, 2019

With the image option set to "full width" and the widget in the Article Bottom widget area, the image is 100% the width of its display: inline anchor tag parent. Which means it's not full width.

Screen Shot 2019-07-09 at 19 09 29

Screen Shot 2019-07-09 at 19 09 19

There's gotta be some CSS we could steal here from the other widget.

@joshdarby
Copy link
Collaborator Author

joshdarby commented Jul 10, 2019

With the image option set to "full width" and the widget in the Article Bottom widget area, the image is 100% the width of its display: inline anchor tag parent. Which means it's not full width.

Screen Shot 2019-07-09 at 19 09 29

Screen Shot 2019-07-09 at 19 09 19

There's gotta be some CSS we could steal here from the other widget.

@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:

.widget.largo-series-posts img {
  max-width: 100px;
}

@joshdarby joshdarby merged commit b9d0d8b into 0.5-dev Jul 12, 2019
@benlk benlk deleted the 1727-add-thumbnail-size-logic-to-series-posts-widget branch July 19, 2019 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Estimate: < 4 Hours priority: high Either blocks work on a priority-normal task or a solution here informs other work. type: feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants