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: News in the client app #34392

Merged
merged 47 commits into from Nov 29, 2018

Conversation

Projects
4 participants
@Bouncey
Copy link
Member

commented Nov 19, 2018

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the master branch of freeCodeCamp.

This PR brings the news app in to the new client.

Currently it builds the 100 latest featured articles, any articles not created by gatsby are fetched and rendered dynamically on the client

@Bouncey Bouncey requested a review from ValeraS Nov 19, 2018

@Bouncey Bouncey requested a review from freeCodeCamp/dev-team as a code owner Nov 19, 2018

@Bouncey Bouncey added this to Critical in Next via automation Nov 19, 2018

@Bouncey Bouncey moved this from Critical to In progress in Next Nov 19, 2018


describe('news-slug integration', () => {
it('returns the correct id from a generated slug', () => {
expect.assertions(100);

This comment has been minimized.

Copy link
@lynxlynxlynx

lynxlynxlynx Nov 19, 2018

Member

Since this is the third use (by my count), maybe a shared named constant is in order. Easier to change if/when that needs to be done.

This comment has been minimized.

Copy link
@Bouncey

Bouncey Nov 19, 2018

Author Member

I don't understand what you mean? expect.assertions is part of the jest API

This comment has been minimized.

Copy link
@lynxlynxlynx

This comment has been minimized.

Copy link
@Bouncey

Bouncey Nov 19, 2018

Author Member

This file is part of a test suite and not used in production, there is no-where else we use the number 100 in a test suite?

Any other instances of 100 in this PR relate to parameters to, and default arguments for the fcc-source-news plugin.

I still don't understand what you mean?

This comment has been minimized.

Copy link
@lynxlynxlynx

lynxlynxlynx Nov 19, 2018

Member

Exactly what you wrote in the second paragraph.

@ValeraS
Copy link
Collaborator

left a comment

I just started, if you wait, I will go on

Show resolved Hide resolved client/plugins/fcc-source-news/gatsby-node.js
Show resolved Hide resolved client/plugins/fcc-source-news/gatsby-node.js Outdated
Show resolved Hide resolved client/utils/news.js Outdated
Show resolved Hide resolved client/src/pages/404.js
Show resolved Hide resolved client/src/pages/404.js Outdated
Show resolved Hide resolved client/src/templates/News/redux/index.js
Show resolved Hide resolved client/src/client-only-routes/ShowDynamicNewsOrFourOhFour.js Outdated
Show resolved Hide resolved client/src/redux/error-saga.js Outdated
@@ -0,0 +1,96 @@
import React from 'react';

This comment has been minimized.

Copy link
@ValeraS

ValeraS Nov 21, 2018

Collaborator

I don't understand how it should work:

  • it shows only 100 articles created by Gatsby and how can I view other articles?
  • it shows all 100 articles immediately.
  • views count is not updated on the page;

This comment has been minimized.

Copy link
@Bouncey

Bouncey Nov 22, 2018

Author Member

You would need to find the shortId of an article not created by gatsby, robo 3T is a good free GUI mongo client that will allow you to run wueries against a mongo instance

This comment has been minimized.

Copy link
@Bouncey

Bouncey Nov 22, 2018

Author Member

I would like to add the idea of infinite scroll when we develop the news application further, for now we only have a small number of articles. It is on the roadmap to allow any use to publish an article, without editors being involved. Editors will still select articles to be featured.

This comment has been minimized.

Copy link
@Bouncey

Bouncey Nov 22, 2018

Author Member

I think this PR is already huge, we can add some sort of view count updating once this has landed

Show resolved Hide resolved client/src/templates/News/NewsReferalLinkHandler/index.js Outdated
Show resolved Hide resolved client/src/templates/News/NewsReferalLinkHandler/index.js Outdated

@Bouncey Bouncey force-pushed the Bouncey:feat/news-to-client branch 2 times, most recently from d85681d to aa7ddaf Nov 25, 2018

@Bouncey Bouncey force-pushed the Bouncey:feat/news-to-client branch from 32f04bc to a02b1ea Nov 28, 2018

@Bouncey Bouncey force-pushed the Bouncey:feat/news-to-client branch from e5f8b55 to 14289f5 Nov 29, 2018

@ValeraS ValeraS merged commit d327a5c into freeCodeCamp:master Nov 29, 2018

1 check passed

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

Next automation moved this from In progress to Done Nov 29, 2018

@Bouncey Bouncey deleted the Bouncey:feat/news-to-client branch Mar 4, 2019

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.