Skip to content
This repository has been archived by the owner on Feb 29, 2020. It is now read-only.

Testing a sponsored section flow #5197

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Testing a sponsored section flow #5197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions common/Reducers.jsm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ const INITIAL_STATE = {
spocs: {
spocs_endpoint: "",
lastUpdated: null,
showSpocs: false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is needed so the jsx gets a better more explicit handle on if spocs are on or not.

data: {}, // {spocs: []}
loaded: false,
frequency_caps: [],
Expand Down Expand Up @@ -606,6 +607,7 @@ function DiscoveryStream(prevState = INITIAL_STATE.DiscoveryStream, action) {
...prevState,
spocs: {
...prevState.spocs,
showSpocs: action.data.showSpocs,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just part of the above.

lastUpdated: action.data.lastUpdated,
data: action.data.spocs,
loaded: true,
Expand Down
41 changes: 34 additions & 7 deletions content-src/components/DiscoveryStreamBase/DiscoveryStreamBase.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { actionCreators as ac } from "common/Actions.jsm";
import { CardGrid } from "content-src/components/DiscoveryStreamComponents/CardGrid/CardGrid";
import { CollapsibleSection } from "content-src/components/CollapsibleSection/CollapsibleSection";
import { connect } from "react-redux";
import { DSDismiss } from "content-src/components/DiscoveryStreamComponents/DSDismiss/DSDismiss";
import { DSMessage } from "content-src/components/DiscoveryStreamComponents/DSMessage/DSMessage";
import { Hero } from "content-src/components/DiscoveryStreamComponents/Hero/Hero";
import { Highlights } from "content-src/components/DiscoveryStreamComponents/Highlights/Highlights";
Expand Down Expand Up @@ -301,11 +302,8 @@ export class _DiscoveryStreamBase extends React.PureComponent {
const styles = [];
return (
<div className="discovery-stream ds-layout">
{layoutRender.map((row, rowIndex) => (
<div
key={`row-${rowIndex}`}
className={`ds-column ds-column-${row.width}`}
>
{layoutRender.map((row, rowIndex) => {
const contents = (
Copy link
Collaborator Author

@ScottDowne ScottDowne Jul 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Changes in this file are wrapping a collection or component if it has a campaign_id in DSDismiss stub.

Copy link
Collaborator

Choose a reason for hiding this comment

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

clarifying collection or component , Only possible component allowed here are LIST, GRID, HERO, COLLECTION ,TEXT BANNER and TOP_SITE or there can be more?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any component can be in a collection, and would be something that if the collection is dismissed, it would take everything with it, so for example, if a collection contains a component that's not something that can be sponsored, for the sake of that collection, it's sponsored because it's used along with a collection that is only sponsored.

In the case of a single component that's sponsored, any component that's got a campaign id (is a promo) would become dismiss-able. I don't think this is something we need to enforce on our end.

A component or collection can be dismiss-able via a campaign id (or some other id) it then wraps it in a dsdismiss component. It's up to the server side data to use these components in a way that creates a logical experience to the user.

<div className="ds-column-grid">
{row.components.map((component, componentIndex) => {
if (!component) {
Expand All @@ -315,15 +313,44 @@ export class _DiscoveryStreamBase extends React.PureComponent {
...(styles[rowIndex] || []),
component.styles,
];
// TODO make this dry
if (component.campaign_id) {
return (
<DSDismiss key={`component-${componentIndex}`} campaignId={component.campaign_id}>
{this.renderComponent(component, row.width)}
</DSDismiss>
);
}
return (
<div key={`component-${componentIndex}`}>
{this.renderComponent(component, row.width)}
</div>
);
})}
</div>
</div>
))}
);
// TODO Dry this out too.
if (row.campaign_id) {
return (
<div
key={`row-${rowIndex}`}
className={`ds-column ds-column-${row.width}`}
>
<DSDismiss campaignId={row.campaign_id}>
{contents}
</DSDismiss>
</div>
);
}
return (
<div
key={`row-${rowIndex}`}
className={`ds-column ds-column-${row.width}`}
>
{contents}
</div>
);
})}
{this.renderStyles(styles)}
</div>
);
Expand Down
21 changes: 21 additions & 0 deletions content-src/components/DiscoveryStreamComponents/DSDismiss/DSDismiss.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

import React from "react";

export class DSDismiss extends React.PureComponent {
render() {
// TODO:
// This needs an x button that dismisses the campaign_id passed to it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clarifying campaign_id, are we saying any LIST, GRID, HERO, COLLECTION TEXT BANNER and TOP_SITE that has campaign_id is promotion?
And if any of these above component has paid record/item (even one) it becomes paid promotion or do we need all items to be sponsored ?
May be @wolasi question, are non-paid promotion dismissable?

Copy link

@wolasi wolasi Jul 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

At this stage a sponsored collection can house content components that either have paid items or non-paid items. We should be able to test both options.

Non-paid collection does not have to be dismissable for v70.

Copy link
Collaborator Author

@ScottDowne ScottDowne Jul 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think the work involved in making non paid dismiss-able is the same, and we should just do both in 70.

I think there may be confusion around the naming of campaign_id. From what I'm seeing, we don't really care if a campaign is paid or not, maybe it makes more sense to change some of the existing names in the code, but how I see it without changing the names in the code.

campaign === promo
sponsored === paid

So if something is sponsored and campaign, it's a paid promo.
If something is campaign and not sponsored, it's a non paid promo.
If something is not a campaign and not sponsored, it's an organic rec.
If something is not a campaign and sponsored, it's a bug and shouldn't happen.

Copy link
Collaborator Author

@ScottDowne ScottDowne Jul 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

To clarify what I meant by "we don't really care if a campaign is paid or not"

If something is a campaign, we make it dismiss-able, at this point we're done, and if it's paid or not, that's different functionality. It's logically split up.

If something is paid (sponsored), we turn it on or off based on if spocs are enabled or not.

We only really need campaign_id, so we can block it based on something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If something is not a campaign and sponsored, it's a bug and shouldn't happen.

if something is not a campaign aka promo and sponsored , isn't that organic sponsored record or same as sponsored story we show now in nightly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

At this stage a sponsored collection can house content components that either have paid items or non-paid items. We should be able to test both options.

@wolasi clarifying your above comment with examples. By house content components you mean e.g. 2 Card Grids, one CardGrid has all paid promo items and second CardGrid has all non-paid promo items. Is there a possibility where one CardGrid will have to display both paid promo and non-paid promo item?

Copy link

Choose a reason for hiding this comment

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

No not quite, here's an example of some different way a collection could be constructed https://www.figma.com/file/eGdZVYlZ5evvePHUCtKnYg7u/New-Tab-Spec-for-Fx-70?node-id=550%3A0

The basic idea is a collection can contain a single content component or a combination of them, with the ability to have paid promo item in the position we want it.

Happy to hop on a call to talk this thru if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that helps, is it possible to update above collection promo figma with spoc UI variants from https://www.figma.com/file/eGdZVYlZ5evvePHUCtKnYg7u/New-Tab-Spec-for-Fx-70?node-id=76%3A167

Copy link

Choose a reason for hiding this comment

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

Done! Is one example enough?

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks @wolasi yes it suffice. @ScottDowne I think current patch doesn't support mixing promo and organic records in a CardGrid component as shown in collection figma. I think we assume all content inside a promo CardGrid component will be promotion only.

// It also needs a hover state for it and its children.
// Somewhere else, we need to filter out anything with a campaign_id that we've blocked.
// Consider calling this collection_id.
// Right now it is this.props.campaignId
return (
<div className="ds-dismiss">
{this.props.children}
</div>
);
}
}
4 changes: 4 additions & 0 deletions content-src/components/DiscoveryStreamComponents/DSDismiss/_DSDismiss.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.ds-dismiss {
background: lightgreen;;
border-radius: 5px;
}
1 change: 1 addition & 0 deletions content-src/components/DiscoveryStreamComponents/DSMessage/DSMessage.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export class DSMessage extends React.PureComponent {
</SafeAnchor>
)}
</header>
{this.props.subtitle && (<div className="subtitle">{this.props.subtitle}</div>)}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just bringing back subtitle of some sort.

</div>
);
}
Expand Down
6 changes: 6 additions & 0 deletions content-src/components/DiscoveryStreamComponents/DSMessage/_DSMessage.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,10 @@
}
}
}

.subtitle {
font-size: 13px;
line-height: 24px;
color: $grey-50;
}
}
22 changes: 16 additions & 6 deletions content-src/lib/selectLayoutRender.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,19 +139,29 @@ export const selectLayoutRender = (state, prefs, rickRollCache) => {
return { ...component, data };
};

const filterComponent = (c) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changes in this file are removing any components or collections that are sponsored if spocs are disabled.

return (
!filterArray.includes(c.type) &&
(!c.sponsored || spocs.showSpocs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah I see you have sponsored property to indicate a campaign is paid promotion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I think one bit that's confusing is anything that comes from the old spocs endpoint, doesn't have the sponsored: true because it explicitly came from a sponsored endpoint so we can assume it's all spocs.

same is true for any feed data coming from a card grid in a sponsored collection. We can assume that's all sponsored content.

Copy link
Collaborator

Choose a reason for hiding this comment

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

is it safe to assume any feed data coming from cardgrid in a sponsored collection, will have context property set , thats used in DSCard UI to display 'Sponsored by ...' label. Just want to highlight that my WIP Card UI variant patch looks for context property against an item and doesn't handle sponsored:true set against a component

);
};

const filterRow = (r) => {
return (
r.components.filter(filterComponent).length &&
(!r.sponsored || spocs.showSpocs)
);
};

const renderLayout = () => {
const renderedLayoutArray = [];
for (const row of layout.filter(
r => r.components.filter(c => !filterArray.includes(c.type)).length
)) {
for (const row of layout.filter(filterRow)) {
let components = [];
renderedLayoutArray.push({
...row,
components,
});
for (const component of row.components.filter(
c => !filterArray.includes(c.type)
)) {
for (const component of row.components.filter(filterComponent)) {
if (component.feed) {
const spocsConfig = component.spocs;
// Are we still waiting on a feed/spocs, render what we have,
Expand Down
1 change: 1 addition & 0 deletions content-src/styles/_activity-stream.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ input {
@import '../components/DiscoveryStreamComponents/DSLinkMenu/DSLinkMenu';
@import '../components/DiscoveryStreamComponents/DSCard/DSCard';
@import '../components/DiscoveryStreamComponents/DSImage/DSImage';
@import '../components/DiscoveryStreamComponents/DSDismiss/DSDismiss';
@import '../components/DiscoveryStreamComponents/DSMessage/DSMessage';
@import '../components/DiscoveryStreamImpressionStats/ImpressionStats';
@import '../components/DiscoveryStreamComponents/DSEmptyState/DSEmptyState';
Expand Down
113 changes: 96 additions & 17 deletions lib/DiscoveryStreamFeed.jsm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,7 @@ this.DiscoveryStreamFeed = class DiscoveryStreamFeed {
sendUpdate({
type: at.DISCOVERY_STREAM_SPOCS_UPDATE,
data: {
showSpocs: this.showSpocs,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is needed to make the jsx better understand if spocs are on or off.

lastUpdated: spocs.lastUpdated,
spocs: newSpocs,
},
Expand Down Expand Up @@ -1286,10 +1287,90 @@ defaultLayoutResp = {
{
width: 12,
components: [
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is all test data.

type: "Message",
header: {
title: "Not Sponsored by Collection Title",
subtitle: "Not Sponsored by Brand Name",
},
properties: {},
},
{
type: "Message",
header: {
title: "Sponsored by Collection Title",
subtitle: "Sponsored by Brand Name",
},
campaign_id: "123456",
sponsored: true,
properties: {},
},
],
},
{
width: 12,
campaign_id: "123456",
components: [
{
type: "Message",
header: {
title: "campaign but not paid",
subtitle: "campaign but not paid",
},
properties: {},
},
{
type: "Message",
header: {
title: "campaign but not paid",
subtitle: "campaign but not paid",
},
properties: {},
},
],
},
{
width: 12,
components: [
{
type: "Message",
campaign_id: "123456",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is Message component has special handling by assigning campaign_id. Clarifying how campaign_id here is different from campaign_id in Line 1356 below

Copy link
Collaborator Author

@ScottDowne ScottDowne Jul 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Line 1356 is to make a whole collection dismiss-able, and everything it wraps, line 1337 is to make just a single component dismiss-able.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the above example of a component being dismiss-able, and that component being a message component, we basically have a wrapper for a promo text component.

header: {
title: "campaign but not paid",
subtitle: "campaign but not paid",
},
properties: {},
},
{
type: "Message",
header: {
title: "not campaign but not paid",
subtitle: "not campaign but not paid",
},
properties: {},
},
],
},
{
width: 12,
campaign_id: "123456",
sponsored: true,
components: [
{
type: "Message",
header: {
title: "Sponsored by Collection Title",
subtitle: "Sponsored by Brand Name",
},
properties: {},
styles: {
".ds-message": "margin-bottom: -28px",
},
},
{
type: "CardGrid",
properties: {
items: 21,
items: 3,
},
header: {
title: "",
Expand All @@ -1299,22 +1380,20 @@ defaultLayoutResp = {
url:
"https://getpocket.cdn.mozilla.net/v3/firefox/global-recs?version=3&consumer_key=$apiKey&locale_lang=en-US&count=30",
},
spocs: {
probability: 1,
positions: [
{
index: 2,
},
{
index: 4,
},
{
index: 11,
},
{
index: 20,
},
],
},
{
type: "Hero",
properties: {
items: 5,
offset: 3,
},
header: {
title: "",
},
feed: {
embed_reference: null,
url:
"https://getpocket.cdn.mozilla.net/v3/firefox/global-recs?version=3&consumer_key=$apiKey&locale_lang=en-US&count=30",
},
},
{
Expand Down