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

@eessex => Article2 Layouts #1713

Merged
merged 11 commits into from Aug 11, 2017

Conversation

Projects
None yet
2 participants
@kanaabe
Contributor

kanaabe commented Aug 10, 2017

Supports classic versions (partner, channel, fair - non-editorial) + AMP as well ---

GIF shows: Standard, feature, small amp update
article2

@kanaabe

This comment has been minimized.

Contributor

kanaabe commented Aug 10, 2017

Not sure why, but the dropcaps/some text stuff isn't showing up in the text component.

@eessex

This comment has been minimized.

Member

eessex commented Aug 11, 2017

The dropcaps are saved into the body html in writer - another item for the backfill list

</div>
</div>
)
return <Article article={this.props.article} />
}

This comment has been minimized.

@eessex

eessex Aug 11, 2017

Member

so nice and clean 👏

@@ -3,29 +3,35 @@ import components from '@artsy/reaction-force/dist/components/publishing/index'
import fixtures from 'desktop/test/helpers/fixtures.coffee'
import React from 'react'

This comment has been minimized.

@eessex

eessex Aug 11, 2017

Member

Not for this PR, but do you think it makes sense to export our reaction fixtures so that we can use identical data for testing across repos?

This comment has been minimized.

@kanaabe

kanaabe Aug 11, 2017

Contributor

Yeah that's a neat idea!

})
it('skips if it has an artwork or image section', (done) => {
const data = {

This comment has been minimized.

@eessex

eessex Aug 11, 2017

Member

Find this test language a little unclear as to what is getting skipped-- Is this to prevent showing images before text?

This comment has been minimized.

@kanaabe

kanaabe Aug 11, 2017

Contributor

Yeah naming could be better -- basically we can't render articles that have artwork or image sections in AMP because they don't have image height + width dimensions saved. Image dimensions are a requirement for rendering images in AMP. See https://github.com/artsy/force/blob/master/desktop/models/article.coffee#L241

credit_line
credit_url
image_url
}
contributing_authors {

This comment has been minimized.

@eessex

eessex Aug 11, 2017

Member

The new postscript field should be included here too

"@storybook/addon-actions@^3.1.8":
version "3.1.8"
resolved "https://registry.yarnpkg.com/@storybook/addon-actions/-/addon-actions-3.1.8.tgz#2b6d7aa97530b19965c1010b822f40b130ebbc4d"
"@storybook/addon-actions@^3.2.0":

This comment has been minimized.

@eessex

eessex Aug 11, 2017

Member

Does reaction require storybook? Seems weird we would need to load this as a production dependency.

This comment has been minimized.

@kanaabe

kanaabe Aug 11, 2017

Contributor

Interesting, it looks like it's added as a dependency instead of a devDependency. I bet this could be moved but I might be missing something. That would be all on the reaction side - worth looking into!

@eessex

This comment has been minimized.

Member

eessex commented Aug 11, 2017

A few minor comments, but this looks great to me. Feel free to merge, looking forward to seeing in action!

@kanaabe kanaabe merged commit 8c4139f into artsy:master Aug 11, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment