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

Conversation

ScottDowne
Copy link
Collaborator

@ScottDowne ScottDowne commented Jul 23, 2019

I added comments to help explain the code.
if you give it a try, I've setup some test data in the hardcoded layout.
what happens in there is trying different combinations of sponsored collections, campaign collections, same with components. If it's a DSDismiss stub, it's wrapped in light green!

@@ -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.

@@ -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.

className={`ds-column ds-column-${row.width}`}
>
{layoutRender.map((row, rowIndex) => {
const contents = (
Copy link
Collaborator Author

@ScottDowne ScottDowne Jul 23, 2019

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.

@@ -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.

@@ -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.

@@ -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.

@@ -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.

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

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

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

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.

const filterComponent = (c) => {
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

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

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.

@punamdahiya
Copy link
Collaborator

Overall this should work, I guess the biggest assumption this approach has is there is no way to individually identify a record is a promotion content or publisher content. All the items in a collection if it has campaign_id are promotion content and we are not allowing a collection e.g. with CardGrid have both promotion content and publisher content in it which definitely simplifies complexity.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants