Skip to content
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

Integrate amp img #1153

Merged
merged 12 commits into from
Jan 14, 2019
Merged

Integrate amp img #1153

merged 12 commits into from
Jan 14, 2019

Conversation

dr3
Copy link

@dr3 dr3 commented Jan 11, 2019

Resolves #953

Integrates amp image in simorgh

  • I have assigned myself to this PR and the corresponding issues
  • Tests added for new features
  • Test engineer approval
  • I have followed the merging checklist and this is ready to merge.

@dr3 dr3 added AMP Work related to AMP alpha-3 labels Jan 11, 2019
@dr3 dr3 self-assigned this Jan 11, 2019
@dr3 dr3 mentioned this pull request Jan 11, 2019
4 tasks
bcmn
bcmn previously approved these changes Jan 11, 2019
Copy link
Contributor

@bcmn bcmn left a comment

Choose a reason for hiding this comment

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

Looks really good, & tested locally 👍

bcmn
bcmn previously approved these changes Jan 14, 2019
bcmn
bcmn previously approved these changes Jan 14, 2019
Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

Please refactor the generateFixtureData function - otherwise looks great! 👍

src/app/containers/Figure/fixtureData.jsx Outdated Show resolved Hide resolved
platform === 'amp' ? (
<AmpImg
alt={alt}
attribution={copyright || ''}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm so this means we'll be adding an amp attribution and rendering the copyright below it. How is attribution actually used? Is it meta information or is it read out to screen readers?

Copy link
Author

Choose a reason for hiding this comment

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

I think its just meta information

Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@jamesbrumpton
Copy link
Contributor

LGTM 👍

@ChrisBAshton ChrisBAshton merged commit ad802e0 into latest Jan 14, 2019
@ChrisBAshton ChrisBAshton deleted the IntegrateAmpImg branch January 14, 2019 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP Work related to AMP high-priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants