Skip to content
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

feat(systemaddon): #2512 Port pocket code to system-addon #2881

Merged
merged 1 commit into from Jul 18, 2017

Conversation

Projects
None yet
5 participants
@csadilek
Copy link
Collaborator

commented Jul 17, 2017

This ports all non-ui code i.e. the feed logic, as discussed, including tests. Section events are based on DummySectionFeed.

@k88hudson Can you take a look, please.

@csadilek csadilek requested a review from k88hudson Jul 17, 2017

@csadilek csadilek force-pushed the csadilek:2512 branch from 2af0fb9 to bb3dc52 Jul 17, 2017

@coveralls

This comment has been minimized.

Copy link

commented Jul 17, 2017

Coverage Status

Coverage remained the same at 87.576% when pulling bb3dc52 on csadilek:2512 into 33cef85 on mozilla:master.

@coveralls

This comment has been minimized.

Copy link

commented Jul 17, 2017

Coverage Status

Coverage remained the same at 87.576% when pulling bb3dc52 on csadilek:2512 into 33cef85 on mozilla:master.

@csadilek csadilek force-pushed the csadilek:2512 branch from bb3dc52 to 3e34975 Jul 17, 2017

@coveralls

This comment has been minimized.

Copy link

commented Jul 17, 2017

Coverage Status

Coverage remained the same at 87.576% when pulling 3e34975 on csadilek:2512 into 33cef85 on mozilla:master.


if (topics) {
this.dispatchUpdateEvent(this.topicsLastUpdated,
{"type": at.SECTION_ROWS_UPDATE, "data": {"id": SECTION_ID, "topics": topics}});

This comment has been minimized.

Copy link
@AdamHillier

AdamHillier Jul 18, 2017

Contributor

This currently won't work as I didn't anticipate anything other than rows being updated with this event but on reflection it's better if it can handle the general case of any data being updated. The fix would be a one-line change in the reducer (line 194).
-- return Object.assign({}, section, {initialized: true, rows: action.data.rows});
++ return Object.assign({}, section, action.data, {initialized: section.rows || action.data.rows});
We can consider renaming the event to SECTION_UPDATEas well but that is less important. Could also put off this fix until we wire all the parts together.

This comment has been minimized.

Copy link
@csadilek

csadilek Jul 18, 2017

Author Collaborator

@AdamHillier done, PR updated. Thanks.

@csadilek csadilek force-pushed the csadilek:2512 branch from 3e34975 to 4882765 Jul 18, 2017

@coveralls

This comment has been minimized.

Copy link

commented Jul 18, 2017

Coverage Status

Coverage remained the same at 87.576% when pulling 4882765 on csadilek:2512 into 33cef85 on mozilla:master.

@csadilek csadilek force-pushed the csadilek:2512 branch from 4882765 to bf8da1c Jul 18, 2017

@coveralls

This comment has been minimized.

Copy link

commented Jul 18, 2017

Coverage Status

Coverage remained the same at 87.576% when pulling bf8da1c on csadilek:2512 into 157fcbc on mozilla:master.

@k88hudson k88hudson self-assigned this Jul 18, 2017

@k88hudson
Copy link
Member

left a comment

Just a few comments, mostly looks great 👍

@@ -192,7 +192,7 @@ function Sections(prevState = INITIAL_STATE.Sections, action) {
case at.SECTION_ROWS_UPDATE:
return prevState.map(section => {
if (section && section.id === action.data.id) {
return Object.assign({}, section, {initialized: true, rows: action.data.rows});
return Object.assign({}, section, action.data, {initialized: section.rows || action.data.rows});

This comment has been minimized.

Copy link
@k88hudson

k88hudson Jul 18, 2017

Member

I would argue a better pattern would be simply using rows in the UI to determine whether to show the empty state or not and to do away with initialized altogether, if that's what this property means;

Generally initialized is useful as a way to differentiate between an initial state from the reducer and a natural empty state (i.e. no items were returned)

This comment has been minimized.

Copy link
@csadilek

csadilek Jul 18, 2017

Author Collaborator

done, we removed initialized, as we won't be using it going forward.

"api_key_pref": "extensions.pocket.oAuthConsumerKey",
"provider_name": "Pocket",
"provider_icon": "pocket.svg"
}`

This comment has been minimized.

Copy link
@k88hudson

async fetchStories() {
if (this.stories_endpoint) {
const stories = await fetch(this.stories_endpoint)

This comment has been minimized.

Copy link
@k88hudson

k88hudson Jul 18, 2017

Member

Ideally this should use async/await here instead of chaining promises, but I'd be ok with a clean-up ticket being filed if you think it would take too long

This comment has been minimized.

Copy link
@csadilek

csadilek Jul 18, 2017

Author Collaborator

OK, we'll refactor this in a follow-up, once everything is working.


_normalizeUrl(url) {
if (url) {
return url.replace(/\(/g, "%28").replace(/\)/g, "%29");

This comment has been minimized.

Copy link
@k88hudson

k88hudson Jul 18, 2017

Member

remind me what this is again?

This comment has been minimized.

Copy link
@csadilek

csadilek Jul 18, 2017

Author Collaborator

added the comment that explains this back in.

@k88hudson k88hudson assigned csadilek and unassigned k88hudson Jul 18, 2017

@csadilek csadilek force-pushed the csadilek:2512 branch from bf8da1c to d2493e8 Jul 18, 2017

@csadilek

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2017

@k88hudson thanks, updates, based on comments.

@coveralls

This comment has been minimized.

Copy link

commented Jul 18, 2017

Coverage Status

Coverage remained the same at 87.576% when pulling d2493e8 on csadilek:2512 into 157fcbc on mozilla:master.

@k88hudson k88hudson assigned k88hudson and unassigned csadilek Jul 18, 2017

@k88hudson k88hudson added the PR / R+ label Jul 18, 2017

@csadilek csadilek merged commit 35b28aa into mozilla:master Jul 18, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 87.576%
Details
@as-pine-proxy

This comment has been minimized.

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