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

Theme support to declare image sizes #17610

Merged
merged 18 commits into from Nov 9, 2017

Conversation

Projects
None yet
3 participants
@mikejolley
Member

mikejolley commented Nov 7, 2017

This closes #17557

Allows image sizes to be defined using add_theme_support. If used, image settings in WooCommerce are hidden. Includes support for all default WP themes.

Themes can pass in the image widths they want to use. e.g.

add_theme_support( 'woocommerce', array(
     'thumbnail_image_width' => 150,
     'single_image_width' => 322,
) );

If present, these settings take priority over settings, and settings are hidden.

There is a new setting called 'thumbnail cropping' which then allows the end user to select:

  • Square crop (1:1)
  • Uncropped
  • Custom aspect ratio

Sizes are then calculated from the given widths.

Single product images are uncropped and not affected by ratio settings.

After changing settings, regenerate thumbnails needs to run or the changes will not be visible. #12880 will handle this.

@mikejolley mikejolley added this to the 3.3.0 milestone Nov 7, 2017

@mikejolley mikejolley requested a review from jameskoster Nov 7, 2017

mikejolley added some commits Nov 7, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 7, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@c4fe292). Click here to learn what that means.
The diff coverage is 5.25%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #17610   +/-   ##
=========================================
  Coverage          ?   34.88%           
  Complexity        ?    12187           
=========================================
  Files             ?      332           
  Lines             ?    49997           
  Branches          ?        0           
=========================================
  Hits              ?    17442           
  Misses            ?    32555           
  Partials          ?        0
Impacted Files Coverage Δ Complexity Δ
...s/api/class-wc-rest-setting-options-controller.php 94.73% <ø> (ø) 50 <0> (?)
includes/class-wc-frontend-scripts.php 0% <ø> (ø) 72 <0> (?)
includes/class-wc-install.php 57.1% <ø> (ø) 101 <0> (?)
includes/class-wc-cart.php 53.05% <ø> (ø) 278 <0> (?)
...ncludes/theme-support/class-wc-twenty-fourteen.php 0% <0%> (ø) 3 <3> (?)
includes/class-wc-geolocation.php 23.56% <0%> (ø) 64 <3> (?)
includes/class-wc-cart-session.php 34.73% <0%> (ø) 31 <0> (?)
includes/admin/wc-admin-functions.php 18.79% <0%> (ø) 0 <0> (?)
...cludes/admin/settings/views/html-webhooks-edit.php 0% <0%> (ø) 0 <0> (?)
includes/theme-support/class-wc-twenty-ten.php 0% <0%> (ø) 3 <3> (?)
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4fe292...b20a43f. Read the comment docs.

codecov bot commented Nov 7, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@c4fe292). Click here to learn what that means.
The diff coverage is 5.25%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #17610   +/-   ##
=========================================
  Coverage          ?   34.88%           
  Complexity        ?    12187           
=========================================
  Files             ?      332           
  Lines             ?    49997           
  Branches          ?        0           
=========================================
  Hits              ?    17442           
  Misses            ?    32555           
  Partials          ?        0
Impacted Files Coverage Δ Complexity Δ
...s/api/class-wc-rest-setting-options-controller.php 94.73% <ø> (ø) 50 <0> (?)
includes/class-wc-frontend-scripts.php 0% <ø> (ø) 72 <0> (?)
includes/class-wc-install.php 57.1% <ø> (ø) 101 <0> (?)
includes/class-wc-cart.php 53.05% <ø> (ø) 278 <0> (?)
...ncludes/theme-support/class-wc-twenty-fourteen.php 0% <0%> (ø) 3 <3> (?)
includes/class-wc-geolocation.php 23.56% <0%> (ø) 64 <3> (?)
includes/class-wc-cart-session.php 34.73% <0%> (ø) 31 <0> (?)
includes/admin/wc-admin-functions.php 18.79% <0%> (ø) 0 <0> (?)
...cludes/admin/settings/views/html-webhooks-edit.php 0% <0%> (ø) 0 <0> (?)
includes/theme-support/class-wc-twenty-ten.php 0% <0%> (ø) 3 <3> (?)
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4fe292...b20a43f. Read the comment docs.

@@ -79,7 +79,7 @@ public static function remove_all_notices() {
* Reset notices for themes when switched or a new version of WC is installed.
*/
public static function reset_admin_notices() {
if ( ! current_theme_supports( 'woocommerce' ) && ! in_array( get_option( 'template' ), wc_get_core_supported_themes() ) ) {
if ( ! current_theme_supports( 'woocommerce' ) ) {
self::add_notice( 'theme_support' );
}

This comment has been minimized.

@claudiulodro

claudiulodro Nov 7, 2017

Contributor

I think we can probably remove this notice entirely if all themes should work with WooCommerce.

@claudiulodro

claudiulodro Nov 7, 2017

Contributor

I think we can probably remove this notice entirely if all themes should work with WooCommerce.

This comment has been minimized.

@claudiulodro

claudiulodro Nov 7, 2017

Contributor

Doesn't need to be in this PR. Just observing.

@claudiulodro

claudiulodro Nov 7, 2017

Contributor

Doesn't need to be in this PR. Just observing.

@claudiulodro

This comment has been minimized.

Show comment
Hide comment
@claudiulodro

claudiulodro Nov 7, 2017

Contributor

This looks good to me. I like the new Twenty_X classes, too. I'll give it a little while for other people to review it then merge if nobody brings up anything.

Contributor

claudiulodro commented Nov 7, 2017

This looks good to me. I like the new Twenty_X classes, too. I'll give it a little while for other people to review it then merge if nobody brings up anything.

@jameskoster

This comment has been minimized.

Show comment
Hide comment
@jameskoster

jameskoster Nov 8, 2017

Member

The preview looks a bit odd on larger and smaller screens. Let's just move it below the settings;

preview

Overall it works great. I'll tweak the preview styling once you've moved it.

Edit: One minor point, would it be possible to update the preview as you update the input rather than when you focus off it? Also noticed it's possible to add decimal values. We should probably not allow that.

Member

jameskoster commented Nov 8, 2017

The preview looks a bit odd on larger and smaller screens. Let's just move it below the settings;

preview

Overall it works great. I'll tweak the preview styling once you've moved it.

Edit: One minor point, would it be possible to update the preview as you update the input rather than when you focus off it? Also noticed it's possible to add decimal values. We should probably not allow that.

mikejolley and others added some commits Nov 8, 2017

@mikejolley mikejolley merged commit 6e08ff0 into master Nov 9, 2017

2 of 5 checks passed

codecov/patch CI failed.
Details
codecov/project CI failed.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Scrutinizer 9 new issues, 4 updated code elements
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mikejolley mikejolley deleted the update/17557 branch Nov 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment