Amp Img #1101
Amp Img #1101
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| const WrappingContainer = ({ alt, height, src, width }) => ( | ||
| <AmpImg alt={alt} src={src} height={height} width={width} /> | ||
| ); | ||
| ``` |
There was a problem hiding this comment.
Needs the rest of the README structure (guess I could have raised an issue for that!), e.g. https://github.com/BBC-News/psammead/tree/latest/packages/components/psammead-paragraph
There was a problem hiding this comment.
The CoC and other links will need to change when we move it to Psammead. I've added a note to the description in this Issue: BBC-archive/psammead#203
Add conditional logic for Img AmpImg rendering in Figure container
|
Visiting a canonical page renders images with Visiting an amp page renders images with |
|
For readability of our test files, I have not refactored all of the tests. |
| width={portrait.width} | ||
| />, | ||
| ); | ||
| shouldMatchSnapshot( |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| import { custom, landscape, portrait, square } from './fixtureData'; | ||
|
|
||
| const snapshotTests = Component => { | ||
| shouldMatchSnapshot( |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| width={landscape.width} | ||
| />, | ||
| ); | ||
| shouldMatchSnapshot( |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| width={square.width} | ||
| />, | ||
| ); | ||
| shouldMatchSnapshot( |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| import { shouldMatchSnapshot } from '../../../../helpers/tests/testHelpers'; | ||
| import { custom, landscape, portrait, square } from './fixtureData'; | ||
|
|
||
| const snapshotTests = Component => { |
There was a problem hiding this comment.
Function snapshotTests has 46 lines of code (exceeds 25 allowed). Consider refactoring.
| width={portrait.width} | ||
| />, | ||
| ); | ||
| shouldMatchSnapshot( |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| import { custom, landscape, portrait, square } from './fixtureData'; | ||
|
|
||
| const snapshotTests = Component => { | ||
| shouldMatchSnapshot( |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| width={landscape.width} | ||
| />, | ||
| ); | ||
| shouldMatchSnapshot( |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| width={square.width} | ||
| />, | ||
| ); | ||
| shouldMatchSnapshot( |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| import { shouldMatchSnapshot } from '../../../../helpers/tests/testHelpers'; | ||
| import { custom, landscape, portrait, square } from './fixtureData'; | ||
|
|
||
| const snapshotTests = Component => { |
There was a problem hiding this comment.
Function snapshotTests has 46 lines of code (exceeds 25 allowed). Consider refactoring.
|
The Option 1 Option 2 It appears that regardless of the layout value passed, the Option 3 This does constrain the image correctly, however the image width won't smoothly transition to be 6 out of 10 columns of our grid. Option 4 |
Don't use ImagePlaceholder and Copyright for AmpImg Use React camelcased srcSet for attribute
…ot pass in height
| {...additionalProps} | ||
| />, | ||
| ); | ||
| shouldMatchSnapshot( |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| import { custom, landscape, portrait, square } from './fixtureData'; | ||
|
|
||
| const snapshotTests = (Component, additionalProps) => { | ||
| shouldMatchSnapshot( |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| {...additionalProps} | ||
| />, | ||
| ); | ||
| shouldMatchSnapshot( |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| import React from 'react'; | ||
| import CanonicalDecorator from '.'; | ||
|
|
||
| describe('CanonicalDecorator', () => { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| {...additionalProps} | ||
| />, | ||
| ); | ||
| shouldMatchSnapshot( |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
|
Code Climate has analyzed commit 974b8ec and detected 9 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 97.3% (90% is the threshold). This pull request will bring the total coverage in the repository to 96.6% (-0.1% change). View more on Code Climate. |
|
Due to #1104 being merged in, the Image component is only in Psammead now. |
Resolves #929
Adding an AmpImg component that extends Img.
Integrating it into FigureContainer.
Testing notes
Visiting a canonical page renders images with
<img>, with attributesalt,src,heightandwidth.http://localhost:7080/news/articles/c9rpqy7pmypo
Visiting an amp page renders images with
<amp-img>, with attributesalt,src,heightandwidth.http://localhost:7080/news/articles/c9rpqy7pmypo.amp
Note, to run the validator and check everything is correct, in one window run:
In the second window run:
(Note: the
npm run test:ampValidateseems to be failing on the wait command. Separate issue to fix this: #1109 )