Skip to content
This repository has been archived by the owner on Jan 10, 2018. It is now read-only.

Add custom sizes attributes to responsive images #254

Merged
merged 5 commits into from Oct 13, 2016

Conversation

mor10
Copy link
Contributor

@mor10 mor10 commented Oct 11, 2016

Adds control of sizes attribute in responsive images to reflect actual displayed size, not only full viewport width. Fixes #120

…l displayed size, not only full viewport width. Handles both content images and featured images. Reworked from Twenty Sixteen.
function twentyseventeen_content_image_sizes_attr( $sizes, $size ) {
$width = $size[0];

//if ( 'page' === get_post_type() || ( ! is_active_sidebar( 'sidebar-1' ) && is_single() ) ) {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Getting a minor formatting error here - No space found before comment text; expected "// if (..."

It looks like it's commented out code - if that's the case, it can just be removed instead. Thanks!

@laurelfulford
Copy link
Collaborator

Thanks for this PR, @mor10!

I noticed a couple small things I wanted to run by you:

First, I get a bit of a gap for image sizes for the 767px breakpoint - it's definitely minor, but the 82vw doesn't seem to cover the whole space all the time:

https://cloudup.com/czv9khWU2q6

What do you think about rolling that one together with the 706px breakpoint in all cases? Instead of having the two for 706px and 767px, it would just be:

(max-width: 767px) 89vw

From what I can tell, 89vw seems to be wide enough to fix the small gap.

Second, when pages are set to use the one column layout in the Customizer, the max image size is 580px:

https://cloudup.com/c-IHFWln_EV

If a check for 'one-column' === get_theme_mod( 'page_options' ) can be incorporated into what you have, with the image size set to 740px for desktop screen sizes (and smaller for responsive), that should do that trick!

Just let me know if you have any questions at all about the above, and thanks again for this work!

… for page layouts and sets the right sizes attributes for featured images on index pages.
@mor10
Copy link
Contributor Author

mor10 commented Oct 13, 2016

@laurelfulford I've updated the PR with the following:

  • Fixed the image width issue you pointed out by simplifying the rules.
  • Added conditional for page layouts with a stacked logic chain. I tried a whole bunch of different iterations and this seems like the cleanest one.
  • Fixed the sizes attribute for featured images on index pages (it was the wrong no-sidebar size in the original PR).


if ( is_active_sidebar( 'sidebar-1' ) || is_archive() || is_search() || is_home() || is_page() ) {
if ( ! ( is_page() && 'one-column' === get_theme_mod( 'twentyseventeen_page_options' ) ) ) {
767 <= $width && $sizes = '(max-width: 767px) 89vw, (max-width: 1000px) 54vw, (max-width: 1071px) 543px, 580px';
Copy link
Collaborator

Choose a reason for hiding this comment

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

The theme_mods had their prefixes removed in #23 - twentyseventeen_page_options just needs to be switched to page_options and this should be good!

@laurelfulford
Copy link
Collaborator

Thanks @mor10! Added a comment for one small issue, but that should be it! 🙂

Also, to verify - is your WordPress.org username mor10 as well?

@mor10
Copy link
Contributor Author

mor10 commented Oct 13, 2016

I'll update the prefix. And yes, my username is mor10 pretty much everywhere.

@laurelfulford laurelfulford merged commit 1202b24 into WordPress:master Oct 13, 2016
@laurelfulford
Copy link
Collaborator

Thanks @mor10!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants