Skip to content

Add PaidForContent TopMeta to Article Body in AMP #513

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

Merged
merged 2 commits into from
Apr 25, 2019
Merged

Conversation

vlbee
Copy link
Member

@vlbee vlbee commented Apr 17, 2019

What does this change?

Adds PaidForTopMeta Component with teal header band and sponser logo to AMP Article Body.

Also extracts tone tag helper functions into tag-utils in lib

Why?

⚡️ AMP completion

Implementation notes:

  • Styling on the body (font, colors, submeta) will be a separate PR
  • The TSLint rule for does not recognize <amp-img> tags as valid inner image tags for anchors and was throwing an error. I have disabled this locally.
return (
        <div>
            <div className={paidForLogoLabelStyle}>Paid for by</div>
            {/* tslint:disable-next-line: react-a11y-anchors */}
            <a
                href={logo.link}
                data-sponsor={sponsorName.toLowerCase()}
                rel="nofollow"
            >
                <amp-img
                    src={logo.src}
                    width="280px"
                    height="180px"
                    alt={sponsorName}
                />
            </a>
        </div>
    );

Screenshots

image

@PRBuilds
Copy link

PRBuilds commented Apr 17, 2019

PRbuilds results:

LightHouse Reporting
1556109331.report.html

--automated message

Copy link
Contributor

@nicl nicl left a comment

Choose a reason for hiding this comment

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

Think this is good. I've left a few comments. But otherwise, it might be worth merging this and then working on the rest? This will make it easier to review later PRs as they'll be smaller and you are less likely to have a merge conflict later. But equally, if you think stuff is still in flux keeping it all in a single PR makes sense!

);
};

export type Tone = 'isOpinion' | 'isPaidContent' | 'isDefault';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is my fault for using Opinion everywhere 🤦‍♂️ but perhaps we should use the actual tone values here (i.e. exactly what the tags contain) - 'comment', 'advertisement-features', '...' ? The benefit is that developers will find it easier to understand the connection between DCR and CAPI/Frontend modelling.

Copy link
Contributor

Choose a reason for hiding this comment

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

The is prefix is probably also worth dropping as types tend to be nouns rather than predicates if that makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed this to match CAPI

@@ -9,6 +9,7 @@ import { AdConsent } from '@frontend/amp/components/AdConsent';
import { css } from 'emotion';
import { Sidebar } from '@frontend/amp/components/Sidebar';
import { Analytics, AnalyticsModel } from '@frontend/amp/components/Analytics';
import { filterForTagsOfType } from '../lib/tag-utils';
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but we tend to use absolute import paths - @frontend/...

We should probably find a way to lint this though! As I think everyone does this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. Will change all as way easier to read as a dev too! Perhaps there is a linter plugin already written?

Looks like there is a TSlint rule that already exists for no-relative-imports that I'll look into: microsoft/tslint-microsoft-contrib#435

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks - sounds great! :)

@vlbee vlbee force-pushed the amp-add-paidfor-tone branch from c19ee9a to 5a8ef13 Compare April 24, 2019 09:55
@vlbee vlbee changed the title Add Article Body styling for PaidContent tone Add PaidForContent TopMeta to Article Body in AMP Apr 24, 2019
@vlbee vlbee force-pushed the amp-add-paidfor-tone branch from f62fe03 to c6daf8e Compare April 25, 2019 09:53
* Remove tagsOfType from Article

* Add getToneTags with tests
@vlbee vlbee force-pushed the amp-add-paidfor-tone branch from c6daf8e to 544492a Compare April 25, 2019 14:06
* Add TopMetaPaidContent component to Body

* Add PaidForBand component to AMP

* Refactor TopMeta by tone in amp Body
@vlbee vlbee force-pushed the amp-add-paidfor-tone branch from 544492a to 8ae8d6b Compare April 25, 2019 14:09
@vlbee vlbee merged commit 9c3a761 into master Apr 25, 2019
@vlbee vlbee deleted the amp-add-paidfor-tone branch May 8, 2019 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants