Fix responsive prop, add a warning when it's not affecting the transformation (#176)

Author: Nir Maoz

e2e-test/cypress/integration/responsive.spec.js

@@ -1,25 +1,44 @@
+/**
+ * When 'responsive' is passed to the Image component, cloudinary-core generates a data-src attribute instead of src.
+ * Then cloudinary.responsive() is called, it will add a 'src' attribute with calculated width according to the size
+ * of the <img> element's container.
+ * In these tests we first test for existence of 'data-src' attribute, and then we check that 'src' was also generated.
+ * Remember that Cypress removes the need to add a wait or timeout, and waits for a test to be true until it's default
+ * timeout is reached.
+ */
 describe('Responsive Image', () => {
   beforeEach(() => {
     // runs before each test in the block
     cy.visit('/');
     cy.get('#responsiveBtn').click();          // Click on button
   });
+  // Here responsive() is called, but since a specified width value was passed (100) alongside responsive,
+  // The responsive() function does not change the width param of the transformation, because the specified width
+  // "overrides" the responsiveness. in other words, specifying a width value is stronger then the 'responsive' prop.
+  it('Should generate transformation with specified width value', () => {
+    cy.get('#responsive-override')
+      .should('have.attr', 'data-src').should('equal', 'http://res.cloudinary.com/demo/image/upload/c_scale,w_100/sample')
+    cy.get('#responsive-override')
+      .should('have.attr', 'src').should('equal', 'http://res.cloudinary.com/demo/image/upload/c_scale,w_100/sample')
+    cy.get('#responsive-override')
+      .should('have.attr', 'width').should('equal', '100')
+  });
   it('Responsive Image', () => {
     cy.get('#responsive')
-      .should('have.attr', 'data-src').should('equal','http://res.cloudinary.com/demo/image/upload/c_scale,w_auto/sample')
+      .should('have.attr', 'data-src').should('equal', 'http://res.cloudinary.com/demo/image/upload/c_scale,w_auto/sample')
     cy.get('#responsive')
-      .should('have.attr', 'src').should('equal','http://res.cloudinary.com/demo/image/upload/c_scale,w_400/sample')
+      .should('have.attr', 'src').should('equal', 'http://res.cloudinary.com/demo/image/upload/c_scale,w_400/sample')
   });
   it('Disabled Breakpoints', () => {
     cy.get('#responsive')
-      .should('have.attr', 'data-src').should('equal','http://res.cloudinary.com/demo/image/upload/c_scale,w_auto/sample')
+      .should('have.attr', 'data-src').should('equal', 'http://res.cloudinary.com/demo/image/upload/c_scale,w_auto/sample')
     cy.get('#disable-breakpoints')
-      .should('have.attr', 'src').should('equal','http://res.cloudinary.com/demo/image/upload/c_scale,w_330/sample')
+      .should('have.attr', 'src').should('equal', 'http://res.cloudinary.com/demo/image/upload/c_scale,w_330/sample')
   });
   it('Enabled Breakpoints', () => {
     cy.get('#responsive')
-      .should('have.attr', 'data-src').should('equal','http://res.cloudinary.com/demo/image/upload/c_scale,w_auto/sample')
+      .should('have.attr', 'data-src').should('equal', 'http://res.cloudinary.com/demo/image/upload/c_scale,w_auto/sample')
     cy.get('#breakpoints')
-      .should('have.attr', 'src').should('equal','http://res.cloudinary.com/demo/image/upload/c_scale,w_450/sample')
+      .should('have.attr', 'src').should('equal', 'http://res.cloudinary.com/demo/image/upload/c_scale,w_450/sample')
   });
 });

e2e-test/src/App.js

@@ -28,6 +28,9 @@ function App() {
       {test === 'responsive' &&
       <Fragment>
         <h1>Responsive Image</h1>
+        <div style={{width: "330px"}}>
+          <Image id="responsive-override" publicId="sample" cloudName="demo" width={100} crop="scale" responsive/>
+        </div>
         <div style={{width: "330px"}}>
           <Image id="responsive" publicId="sample" cloudName="demo" width="auto" crop="scale" responsive/>
         </div>

package.json

@@ -57,7 +57,7 @@
     "webpack-cli": "^3.1.2"
   },
   "dependencies": {
-    "cloudinary-core": "^2.10.2",
+    "cloudinary-core": "^2.10.3",
     "prop-types": "^15.6.2"
   },
   "peerDependencies": {

src/components/Image/Image.js

@@ -1,9 +1,17 @@
 import React, {createRef, Fragment} from 'react';
 import CloudinaryComponent from '../CloudinaryComponent';
-import {extractCloudinaryProps, getImageTag, makeElementResponsive, getConfiguredCloudinary} from "../../Util";
+import {extractCloudinaryProps, getImageTag, makeElementResponsive} from "../../Util";
 import {Util} from "cloudinary-core";
 import PropTypes from "prop-types";
 
+const RESPONSIVE_OVERRIDE_WARNING = [
+  `Warning: passing a number value for width cancels the 'responsive' prop's effect on the image transformation.`,
+  `The 'responsive' prop affects the image transformation only when width === 'auto'.`,
+  `Passing 'width="auto" responsive' will affect the actual image width that is fetched from Cloudinary.`,
+  `The 'responsive' prop causes the Image component to request an image which width is equal to the width of it's container.`,
+  `When passing 'width="auto" responsive', you can set the <img> element width by passing a 'style' prop`
+].join('\n');
+
 /**
  * A component representing a Cloudinary served image
  */
@@ -19,7 +27,12 @@ class Image extends CloudinaryComponent {
    * @return true when this image element should be made responsive, false otherwise.
    */
   isResponsive = () => {
-    return this.props.responsive && this.imgElement && this.imgElement.current;
+    const {responsive, width} = this.getExtendedProps();
+    if (responsive && width !== 'auto') {
+      console.warn(RESPONSIVE_OVERRIDE_WARNING);
+    }
+
+    return responsive && this.imgElement && this.imgElement.current;
   }
 
   /**
@@ -44,33 +57,46 @@ class Image extends CloudinaryComponent {
 
     let attributes = {...getImageTag(options).attributes(), ...nonCloudinaryProps};
 
+    //React requires camelCase instead of snake_case attributes
+    attributes = Util.withCamelCaseKeys(attributes);
+
     // Set placeholder Id
     if (placeholder && attributes.id) {
       attributes.id = attributes.id + '-cld-placeholder';
     }
 
+    // Set dataSrc if lazy loading and not in view
+    if (!isInView && this.shouldLazyLoad(options)) {
+      attributes.dataSrc = attributes.dataSrc || attributes.src;
+      delete attributes.src;
+    }
+
+    // The data-src attribute was turned into dataSrc by the camelCase function,
+    // But it's needed by cloudinary-core's responsive() function. Notice that it's not snake_case.
+    if (attributes.dataSrc) {
+      attributes['data-src'] = attributes.dataSrc;
+    }
+
     // Remove unneeded attributes,
-    ["src", "accessibility", "placeholder"].forEach(attr => {
+    ['dataSrc', 'accessibility', 'placeholder', 'breakpoints'].forEach(attr => {
       delete attributes[attr];
     });
 
-    // Set src or data-src attribute
-    const srcAttrName = isInView || !this.shouldLazyLoad(options) ? "src" : "data-src";
-    attributes[srcAttrName] = getConfiguredCloudinary(options).url(options.publicId, options);
-
     return attributes;
   }
 
   /**
    * Update this image using cloudinary-core
    */
   update = () => {
+    const {isInView} = this.state;
+
     if (this.isResponsive()) {
       const removeListener = makeElementResponsive(this.imgElement.current, this.getOptions());
       this.listenerRemovers.push(removeListener);
     }
 
-    if (this.shouldLazyLoad(this.getExtendedProps())) {
+    if (!isInView && this.shouldLazyLoad(this.getExtendedProps())) {
       Util.detectIntersection(this.imgElement.current, this.onIntersect);
     }
   }
@@ -153,7 +179,6 @@ class Image extends CloudinaryComponent {
         </Fragment>
       );
     }
-
     return <img ref={attachRef} {...attributes}/>
   }
 }

test/ImageTest.js

@@ -94,11 +94,6 @@ describe('Image', () => {
 
     expect(tag.find('img').prop('src')).to.match(/fn_wasm:blur.wasm\/sample/);
   });
-  it('should support responsive prop', () => {
-    let tag = mount(<Image publicId="sample" cloudName="demo"/>);
-    tag.setProps({responsive: true});
-    expect(tag.find('img').prop('data-src')).to.equal('http://res.cloudinary.com/demo/image/upload/sample');
-  });
   it('Should support forwarding innerRef to underlying image element', function () {
     const expected = 'http://res.cloudinary.com/demo/image/upload/sample';
     let myRef = React.createRef();
@@ -129,6 +124,17 @@ describe('Image', () => {
 
     expect(tag.find('img').prop('src')).to.equal(expected);
   });
+  describe('Responsive', () => {
+    it('should support responsive prop', () => {
+      let tag = mount(<Image publicId="sample" cloudName="demo"/>);
+      tag.setProps({responsive: true});
+      expect(tag.find('img').prop('data-src')).to.equal('http://res.cloudinary.com/demo/image/upload/sample');
+    });
+    it('should set image width even when responsive prop is passed', () => {
+      let tag = mount(<Image publicId="sample" cloudName="demo" width={100} responsive/>);
+      expect(tag.find('img').prop('width')).to.equal(100);
+    });
+  });
   describe('Placeholder', () => {
     it(`should not have opacity and position when placeholder doesn't exists`, () => {
       const tag = shallow(<Image publicId="sample" cloudName="demo"/>);
@@ -185,7 +191,7 @@ describe('Image', () => {
     });
     describe('Responsive Placeholder With Lazy Loading', () => {
       let tag = shallow(
-        <Image publicId="sample" cloudName="demo" loading="lazy" responsive width="auto" crop="scale">
+        <Image publicId="sample" cloudName="demo" loading="lazy" width="auto" crop="scale" responsive>
           <Placeholder/>
         </Image>
       );