From acfa9140fbf90b8f1ec60c1b6f0a23cffec7f1f6 Mon Sep 17 00:00:00 2001 From: maoznir Date: Mon, 27 Jul 2020 15:22:00 +0300 Subject: [PATCH 1/5] Fix responsive placeholder --- .../cypress/integration/placeholder.spec.js | 4 +- .../integration/responsivePlaceholder.spec.js | 19 +++++++++ e2e-test/src/App.js | 14 ++++++- src/components/Image/Image.js | 42 ++++++++++++++----- test/ImageTest.js | 15 +++++++ 5 files changed, 80 insertions(+), 14 deletions(-) create mode 100644 e2e-test/cypress/integration/responsivePlaceholder.spec.js diff --git a/e2e-test/cypress/integration/placeholder.spec.js b/e2e-test/cypress/integration/placeholder.spec.js index c64c4b4..0d696ff 100644 --- a/e2e-test/cypress/integration/placeholder.spec.js +++ b/e2e-test/cypress/integration/placeholder.spec.js @@ -4,10 +4,10 @@ describe('Placeholder Image', () => { cy.visit('/'); cy.get('#placeholderBtn').click(); }); - + /** * Cypress seems to be not fast enough to catch the placeholder render. - * So we test that the placeholder is rendered in out Unit Tests, + * So we test that the placeholder is rendered in our Unit Tests, * And here we make sure that eventually we render the original image. */ it('Show original image', () => { diff --git a/e2e-test/cypress/integration/responsivePlaceholder.spec.js b/e2e-test/cypress/integration/responsivePlaceholder.spec.js new file mode 100644 index 0000000..82da1d3 --- /dev/null +++ b/e2e-test/cypress/integration/responsivePlaceholder.spec.js @@ -0,0 +1,19 @@ +describe('Responsive Placeholder Image', () => { + beforeEach(() => { + // runs before each test in the block + cy.visit('/'); + cy.get('#responsivePlaceholderBtn').click(); + }); + + /** + * Cypress seems to be not fast enough to catch the placeholder render. + * So we test that the placeholder is rendered in our Unit Tests, + * And here we make sure that eventually we render the responsive image. + * The image width was updated to be w_400. + */ + it('Show original image', () => { + cy.get('#responsivePlaceholder') + .should('be.visible') + .should('have.attr', 'src').should('equal', 'http://res.cloudinary.com/demo/image/upload/c_scale,w_400/sample') + }); +}); \ No newline at end of file diff --git a/e2e-test/src/App.js b/e2e-test/src/App.js index e584eef..f2a106a 100644 --- a/e2e-test/src/App.js +++ b/e2e-test/src/App.js @@ -7,7 +7,8 @@ const tests = [ 'placeholder', 'lazy', 'lazyPlaceholder', - 'lazyResponsive' + 'lazyResponsive', + 'responsivePlaceholder' ]; function App() { @@ -83,6 +84,17 @@ function App() { } + } + {test === 'responsivePlaceholder' && +
+

Responsive Placeholder

+
+ + + +
+
+ } ); } diff --git a/src/components/Image/Image.js b/src/components/Image/Image.js index acc0d4f..ed58e23 100644 --- a/src/components/Image/Image.js +++ b/src/components/Image/Image.js @@ -19,6 +19,7 @@ class Image extends CloudinaryComponent { constructor(props, context) { super(props, context); this.imgElement = createRef(); + this.placeholderElement = createRef(); this.state = {isLoaded: false} this.listenerRemovers = []; } @@ -42,7 +43,7 @@ class Image extends CloudinaryComponent { const extendedProps = this.getExtendedProps(); const {children, innerRef, ...options} = {...extendedProps, ...this.getTransformation(extendedProps)}; - if (!this.shouldLazyLoad()){ + if (!this.shouldLazyLoad()) { delete options.loading; } @@ -80,7 +81,7 @@ class Image extends CloudinaryComponent { // Remove unneeded attributes, ['dataSrc', 'accessibility', 'placeholder', 'breakpoints'].forEach(attr => { delete attributes[attr]; - }); + }) return attributes; } @@ -96,8 +97,17 @@ class Image extends CloudinaryComponent { } else { // Handle responsive only if lazy loading wasn't requested or already handled if (this.isResponsive()) { - const removeListener = makeElementResponsive(this.imgElement.current, this.getOptions()); - this.listenerRemovers.push(removeListener); + const options = this.getOptions(); + const placeholder = this.getPlaceholderType(); + + if (placeholder) { + const placeholderOptions = {...options, placeholder}; + const removePlaceholderListener = makeElementResponsive(this.placeholderElement.current, placeholderOptions); + this.listenerRemovers.push(removePlaceholderListener); + } + + const removeImgListener = makeElementResponsive(this.imgElement.current, options); + this.listenerRemovers.push(removeImgListener); } } } @@ -158,35 +168,45 @@ class Image extends CloudinaryComponent { }); }; + renderPlaceholder = (placeholder, attributes) => { attributes.style = {...(attributes.style || {}), opacity: 0, position: 'absolute'} attributes.onLoad = this.handleImageLoaded; const placeholderWrapperStyle = {display: 'inline'} - const placeholderAttributes = this.getAttributes({placeholder: placeholder.props.type}); + const placeholderAttributes = this.getAttributes({placeholder}); return ( {this.renderImage(attributes)}
- +
); }; renderImage = (attributes) => { - return + return } - render() { - const {isLoaded} = this.state; + getPlaceholderType = () => { const {children} = this.getExtendedProps(); const placeholder = this.getChildPlaceholder(children); + if (!placeholder) { + return null; + } + + return placeholder.props.type; + } + + render() { + const {isLoaded} = this.state; const attributes = this.getAttributes(); + const placeholderType = this.getPlaceholderType(); //If image wasn't loaded and there's a child placeholder then we render it. - if (!isLoaded && placeholder) { - return this.renderPlaceholder(placeholder, attributes); + if (!isLoaded && placeholderType) { + return this.renderPlaceholder(placeholderType, attributes); } return this.renderImage(attributes); diff --git a/test/ImageTest.js b/test/ImageTest.js index 7dcfe87..b0ced9e 100644 --- a/test/ImageTest.js +++ b/test/ImageTest.js @@ -205,6 +205,21 @@ describe('Image', () => { ].join('')); }); }); + describe('Responsive Placeholder', () => { + let tag = shallow( + + + + ); + it('should have data-src for placeholder and image', function () { + expect(tag.html()).to.equal([ + ``, + `
`, + ``, + `
` + ].join('')); + }); + }); describe('Responsive Placeholder With Lazy Loading', () => { let tag = shallow( From 55e81adce4eaaae9110b3c55340e56fe62c8ff44 Mon Sep 17 00:00:00 2001 From: maoznir Date: Mon, 27 Jul 2020 15:23:32 +0300 Subject: [PATCH 2/5] Remove image key --- src/components/Image/Image.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Image/Image.js b/src/components/Image/Image.js index ed58e23..b6fdd81 100644 --- a/src/components/Image/Image.js +++ b/src/components/Image/Image.js @@ -186,7 +186,7 @@ class Image extends CloudinaryComponent { }; renderImage = (attributes) => { - return + return } getPlaceholderType = () => { From 0438307f4ae4ed5b922a1384d5db0ce28854688a Mon Sep 17 00:00:00 2001 From: maoznir Date: Mon, 27 Jul 2020 15:25:02 +0300 Subject: [PATCH 3/5] Update render() param names --- src/components/Image/Image.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/Image/Image.js b/src/components/Image/Image.js index b6fdd81..80a9e02 100644 --- a/src/components/Image/Image.js +++ b/src/components/Image/Image.js @@ -202,11 +202,11 @@ class Image extends CloudinaryComponent { render() { const {isLoaded} = this.state; const attributes = this.getAttributes(); - const placeholderType = this.getPlaceholderType(); + const placeholder = this.getPlaceholderType(); //If image wasn't loaded and there's a child placeholder then we render it. - if (!isLoaded && placeholderType) { - return this.renderPlaceholder(placeholderType, attributes); + if (!isLoaded && placeholder) { + return this.renderPlaceholder(placeholder, attributes); } return this.renderImage(attributes); From d53bb62d66fdf292701d9a64daa5d31d480ae630 Mon Sep 17 00:00:00 2001 From: maoznir Date: Mon, 27 Jul 2020 15:26:27 +0300 Subject: [PATCH 4/5] Update getPlaceholderType --- src/components/Image/Image.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/components/Image/Image.js b/src/components/Image/Image.js index 80a9e02..fcd22dd 100644 --- a/src/components/Image/Image.js +++ b/src/components/Image/Image.js @@ -192,11 +192,8 @@ class Image extends CloudinaryComponent { getPlaceholderType = () => { const {children} = this.getExtendedProps(); const placeholder = this.getChildPlaceholder(children); - if (!placeholder) { - return null; - } - - return placeholder.props.type; + + return placeholder ? placeholder.props.type : null; } render() { From e257f65fd557cbd4d6bb3e56bab5837dd1f2c8ac Mon Sep 17 00:00:00 2001 From: maoznir Date: Mon, 27 Jul 2020 15:29:50 +0300 Subject: [PATCH 5/5] Add update() documentation --- src/components/Image/Image.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/components/Image/Image.js b/src/components/Image/Image.js index fcd22dd..f26c8eb 100644 --- a/src/components/Image/Image.js +++ b/src/components/Image/Image.js @@ -43,7 +43,7 @@ class Image extends CloudinaryComponent { const extendedProps = this.getExtendedProps(); const {children, innerRef, ...options} = {...extendedProps, ...this.getTransformation(extendedProps)}; - if (!this.shouldLazyLoad()) { + if (!this.shouldLazyLoad()){ delete options.loading; } @@ -81,7 +81,7 @@ class Image extends CloudinaryComponent { // Remove unneeded attributes, ['dataSrc', 'accessibility', 'placeholder', 'breakpoints'].forEach(attr => { delete attributes[attr]; - }) + }); return attributes; } @@ -100,12 +100,13 @@ class Image extends CloudinaryComponent { const options = this.getOptions(); const placeholder = this.getPlaceholderType(); + // Make placeholder responsive if (placeholder) { - const placeholderOptions = {...options, placeholder}; - const removePlaceholderListener = makeElementResponsive(this.placeholderElement.current, placeholderOptions); + const removePlaceholderListener = makeElementResponsive(this.placeholderElement.current, {...options, placeholder}); this.listenerRemovers.push(removePlaceholderListener); } + // Make original image responsive const removeImgListener = makeElementResponsive(this.imgElement.current, options); this.listenerRemovers.push(removeImgListener); } @@ -192,7 +193,7 @@ class Image extends CloudinaryComponent { getPlaceholderType = () => { const {children} = this.getExtendedProps(); const placeholder = this.getChildPlaceholder(children); - + return placeholder ? placeholder.props.type : null; }