Skip to content
This repository has been archived by the owner. It is now read-only.

Display pretty name for sources if available #93

Merged
merged 4 commits into from Oct 10, 2017

Conversation

Projects
None yet
3 participants
@c-w
Copy link
Member

commented Oct 9, 2017

As requested by @dmakogon, here's a change to use the trusted source display name (instead of id) when rendering.

Note that this is an optional UI improvement operation (not critical path) so if looking up the trusted sources display names fails, we still proceed with loading the sentences: it's better to show the sentences with an ugly id than not at all.

Screenshot showing result of new logic to replace Facebook page id with page name:

image

Display pretty name for sources if available
Note that this is an optional UI improvement operation (not critical
path) so if looking up the trusted sources display names fails, we still
proceed with loading the sentences: it's better to show the sentences
with an ugly id than not at all!

@c-w c-w requested a review from dmakogon Oct 9, 2017

@@ -52,7 +67,18 @@ export default class ActivityFeed extends React.Component {
const externalsourceid = props.externalsourceid !== constants.DEFAULT_EXTERNAL_SOURCE ? props.externalsourceid : null;
const fulltextTerm = "";

SERVICES.FetchMessageSentences(externalsourceid, bbox, zoomLevel, fromDate, toDate, ActivityConsts.OFFSET_INCREMENT, pageState, [maintopic].concat(Array.from(termFilters)), pipelinekeys, fulltextTerm, callback);
ADMIN_SERVICES.fetchTrustedSources(pipelinekeys, "", (trustedSourcesErr, trustedSourcesResponse, trustedSourcesBody) => {

This comment has been minimized.

Copy link
@erikschlegel

erikschlegel Oct 9, 2017

Collaborator

Move the trusted source service lookup to the dashboard action and bind to the DataStore. The trusted source lookup should take place when the dashboard is initially loaded and when we load the enabled streams, keyword list, etc.

We want to avoid this as this lookup will by invoked for each page scroll in the news feed component. Also, trusted sources will need to be used not only for activity feed, but the popular sources chart, the source filter typeahead control and the trusted source list editor in the admin module.

This comment has been minimized.

Copy link
@c-w

c-w Oct 10, 2017

Author Member

I wanted to isolate this change as much as possible in order to reduce the risk, but since there is already another place that reaches into the admin services to make this same call, maybe it's indeed time to pull this up.

This comment has been minimized.

Copy link
@c-w

c-w Oct 10, 2017

Author Member

Alright, @erikschlegel, I pushed the trusted sources fetching out of the ActivityFeed component into the dashboard initialization (so we only do it once) in 33ae6ad and then also refactored the TypeaheadSearch to avoid re-fetching the trusted sources in ed56620.

@jcjimenez
Copy link

left a comment

LGTM

@c-w

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2017

Updated screenshots after this morning's changes:

ActivityFeed component:

image

TypeaheadSearch component:

image

@erikschlegel
Copy link
Collaborator

left a comment

LGTM

@c-w c-w merged commit 605357e into master Oct 10, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@c-w c-w deleted the facebookexternalsources-displayname branch Oct 10, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.