-
Notifications
You must be signed in to change notification settings - Fork 113
feat(systemaddon): Add HTTP Referrer to Top Stories #2959
Conversation
@@ -27,6 +29,10 @@ class Card extends React.Component { | |||
showContextMenu: true | |||
}); | |||
} | |||
onClick(event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Probably something like onLinkClick
as there's multiple "click" events going on here. See
onLinkClick() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I agree, that would be clearer. Also can you add a comment somewhere around here about why this exists? (i.e. that we need to override the regular behaviour because of HTTP referrer stuff)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, updated.
@@ -207,9 +215,23 @@ class PlacesFeed { | |||
case at.DELETE_HISTORY_URL: | |||
NewTabUtils.activityStreamLinks.deleteHistoryEntry(action.data); | |||
break; | |||
case at.OPEN_NEW_WINDOW: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that TopSitesFeed
was an odd place to have had this, but I'm not sure PlacesFeed
is that much more appropriate… but I suppose that's @k88hudson's call to make!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be inclined to suggest creating a new WindowFeed
or something like that... but because there is a lot of overhead to creating new jsms, maybe just leave this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just a couple of small comments 👍
@@ -27,6 +29,10 @@ class Card extends React.Component { | |||
showContextMenu: true | |||
}); | |||
} | |||
onClick(event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I agree, that would be clearer. Also can you add a comment somewhere around here about why this exists? (i.e. that we need to override the regular behaviour because of HTTP referrer stuff)
@@ -207,9 +215,23 @@ class PlacesFeed { | |||
case at.DELETE_HISTORY_URL: | |||
NewTabUtils.activityStreamLinks.deleteHistoryEntry(action.data); | |||
break; | |||
case at.OPEN_NEW_WINDOW: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be inclined to suggest creating a new WindowFeed
or something like that... but because there is a lot of overhead to creating new jsms, maybe just leave this for now.
Adding the referrer header when clicking on a card, as well as "open new window" and "open new private window" of the card's context menu.
Moved the handling of OPEN_NEW_WINDOW to the PlacesFeed as it's used by both TopSites and TopStories.