diff --git a/src/app/Layouts/defaultPageWrapper.jsx b/src/app/Layouts/defaultPageWrapper.jsx new file mode 100644 index 00000000000..fb7c51020c7 --- /dev/null +++ b/src/app/Layouts/defaultPageWrapper.jsx @@ -0,0 +1,44 @@ +import React, { Fragment } from 'react'; +import { node, string, bool } from 'prop-types'; +import Helmet from 'react-helmet'; +import HeaderContainer from '../containers/Header'; +import FooterContainer from '../containers/Footer'; +import { ServiceContextProvider } from '../contexts/ServiceContext'; +import { RequestContextProvider } from '../contexts/RequestContext'; +import GlobalStyle from '../lib/globalStyles'; +import ConsentBanner from '../containers/ConsentBanner'; + +const PageWrapper = ({ bbcOrigin, children, service, isAmp }) => ( + + + + + + + + + + {children} + + + + +); + +PageWrapper.propTypes = { + bbcOrigin: string, + children: node.isRequired, + service: string.isRequired, + isAmp: bool.isRequired, +}; + +PageWrapper.defaultProps = { + bbcOrigin: null, +}; + +PageWrapper.defaultProps = {}; + +export default PageWrapper; diff --git a/src/app/containers/Article/__snapshots__/index.test.jsx.snap b/src/app/containers/Article/__snapshots__/index.test.jsx.snap index b88b21f3582..6a1508ac8cd 100644 --- a/src/app/containers/Article/__snapshots__/index.test.jsx.snap +++ b/src/app/containers/Article/__snapshots__/index.test.jsx.snap @@ -1,324 +1,23 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`ArticleContainer Component 200 status code should render correctly for news 1`] = ` - - - +
- - - - - - - - - - - -`; - -exports[`ArticleContainer Component 200 status code should render correctly for persian 1`] = ` - - - - - - - - - - - - - - -`; - -exports[`ArticleContainer Component no data or bbcOrigin should render correctly 1`] = `null`; - -exports[`ArticleContainer Component non-200 status code should render correctly 1`] = ` - - - - - - - - - - - - - - +
+ ArticleMain +
+
+ + + `; diff --git a/src/app/containers/Article/index.jsx b/src/app/containers/Article/index.jsx index 130bcdd9d97..47ec421963e 100644 --- a/src/app/containers/Article/index.jsx +++ b/src/app/containers/Article/index.jsx @@ -1,84 +1,31 @@ -import React, { Fragment } from 'react'; -import { bool, string, shape } from 'prop-types'; -import Helmet from 'react-helmet'; -import HeaderContainer from '../Header'; -import FooterContainer from '../Footer'; +import React from 'react'; +import { shape } from 'prop-types'; +import compose from '../../helpers/compose'; import articlePropTypes from '../../models/propTypes/article'; -import { ServiceContextProvider } from '../../contexts/ServiceContext'; -import { RequestContextProvider } from '../../contexts/RequestContext'; -import GlobalStyle from '../../lib/globalStyles'; import ArticleMain from '../ArticleMain'; -import ErrorMain from '../ErrorMain'; -import nodeLogger from '../../helpers/logger.node'; -import ConsentBanner from '../ConsentBanner'; -const logger = nodeLogger(__filename); +import withPageWrapper from '../PageHandlers/withPageWrapper'; +import withError from '../PageHandlers/withError'; +import withLoading from '../PageHandlers/withLoading'; +import withData from '../PageHandlers/withData'; -/* - [1] This handles async data fetching, and a 'loading state', which we should look to handle more intelligently. -*/ -const ArticleContainer = ({ - loading, - error, - data, - bbcOrigin, - isAmp, - service, -}) => { - if (loading) return 'Loading...'; /* [1] */ - if (error) { - logger.error(error); - return 'Something went wrong :('; - } - - if (data) { - const { pageData, status } = data; - - return ( - - - - - - - - - - {status === 200 ? ( - - ) : ( - - )} - - - - - ); - } - - return null; -}; +const ArticleContainer = ({ pageData }) => ( + +); ArticleContainer.propTypes = { - loading: bool, - error: string, - data: shape(articlePropTypes), - bbcOrigin: string, - isAmp: bool.isRequired, - service: string.isRequired, + pageData: shape(articlePropTypes), }; ArticleContainer.defaultProps = { - loading: false, - error: null, - data: null, - bbcOrigin: null, + pageData: null, }; -export default ArticleContainer; +const EnhancedArticleContainer = compose( + withPageWrapper, + withLoading, + withError, + withData, +)(ArticleContainer); + +export default EnhancedArticleContainer; diff --git a/src/app/containers/Article/index.test.jsx b/src/app/containers/Article/index.test.jsx index c37767fe64e..a19c5335108 100644 --- a/src/app/containers/Article/index.test.jsx +++ b/src/app/containers/Article/index.test.jsx @@ -1,63 +1,61 @@ import React from 'react'; -import { shouldShallowMatchSnapshot } from '../../helpers/tests/testHelpers'; +import { shouldMatchSnapshot } from '../../helpers/tests/testHelpers'; import ArticleContainer from './index'; -import { articleDataNews, articleDataPersian } from './fixtureData'; // explicitly ignore console.log errors for Article/index:getInitialProps() error logging global.console.log = jest.fn(); -describe('ArticleContainer', () => { - const newsProps = { - data: { - pageData: articleDataNews, - status: 200, - }, - isAmp: false, - service: 'news', - }; +jest.mock('../PageHandlers/withPageWrapper', () => Component => { + const PageWrapperContainer = props => ( +
+ +
+ ); - const persianProps = { - data: { - pageData: articleDataPersian, - status: 200, - }, - isAmp: false, - service: 'persian', - }; + return PageWrapperContainer; +}); - const badData = { - data: { - pageData: undefined, - status: 451, - }, - isAmp: false, - service: 'news', - }; +jest.mock('../PageHandlers/withLoading', () => Component => { + const LoadingContainer = props => ( +
+ +
+ ); - const bbcOrigin = 'https://www.bbc.co.uk'; + return LoadingContainer; +}); - describe('Component', () => { - describe('200 status code', () => { - shouldShallowMatchSnapshot( - 'should render correctly for news', - , - ); - shouldShallowMatchSnapshot( - 'should render correctly for persian', - , - ); - }); +jest.mock('../PageHandlers/withError', () => Component => { + const ErrorContainer = props => ( +
+ +
+ ); - describe('non-200 status code', () => { - shouldShallowMatchSnapshot( - 'should render correctly', - , - ); - }); + return ErrorContainer; +}); + +jest.mock('../PageHandlers/withData', () => Component => { + const DataContainer = props => ( +
+ +
+ ); + + return DataContainer; +}); + +jest.mock('../ArticleMain', () => { + const ArticleMain = () =>
ArticleMain
; - describe('no data or bbcOrigin', () => { - shouldShallowMatchSnapshot( - 'should render correctly', + return ArticleMain; +}); + +describe('ArticleContainer', () => { + describe('Component', () => { + describe('Composing the Article Container using the page handlers', () => { + shouldMatchSnapshot( + 'should compose articleContainer with the Page Handler in the correct order', , ); }); diff --git a/src/app/containers/PageHandlers/__snapshots__/withData.test.jsx.snap b/src/app/containers/PageHandlers/__snapshots__/withData.test.jsx.snap new file mode 100644 index 00000000000..4dd169d00fe --- /dev/null +++ b/src/app/containers/PageHandlers/__snapshots__/withData.test.jsx.snap @@ -0,0 +1,162 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`withData HOC with missing articleData should return the errorMain component 1`] = ` + +`; + +exports[`withData HOC with no data should return the errorMain component and 500 status 1`] = ` + +`; + +exports[`withData HOC with non 200 status should return the errorMain component 1`] = ` + +`; + +exports[`withData HOC with valid props should return the passed in component 1`] = ` + +`; diff --git a/src/app/containers/PageHandlers/__snapshots__/withError.test.jsx.snap b/src/app/containers/PageHandlers/__snapshots__/withError.test.jsx.snap new file mode 100644 index 00000000000..c53f5153884 --- /dev/null +++ b/src/app/containers/PageHandlers/__snapshots__/withError.test.jsx.snap @@ -0,0 +1,9 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`withError HOC with error should return the errorMain component 1`] = ` + +`; + +exports[`withError HOC with no error should return the passed in component 1`] = ``; diff --git a/src/app/containers/PageHandlers/__snapshots__/withLoading.test.jsx.snap b/src/app/containers/PageHandlers/__snapshots__/withLoading.test.jsx.snap new file mode 100644 index 00000000000..e39877cb92f --- /dev/null +++ b/src/app/containers/PageHandlers/__snapshots__/withLoading.test.jsx.snap @@ -0,0 +1,13 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`withLoading HOC and no loading prop should return the passed in component 1`] = ``; + +exports[`withLoading HOC and the loading prop set to true should return the loading component 1`] = ` +
+ + + +
+`; diff --git a/src/app/containers/PageHandlers/__snapshots__/withPageWrapper.test.jsx.snap b/src/app/containers/PageHandlers/__snapshots__/withPageWrapper.test.jsx.snap new file mode 100644 index 00000000000..c46a8dff01b --- /dev/null +++ b/src/app/containers/PageHandlers/__snapshots__/withPageWrapper.test.jsx.snap @@ -0,0 +1,357 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`with pageWarpper should render correctly 1`] = ` +Array [ + .c0 { + background-color: #B80000; + height: 5rem; + width: 100%; +} + +.c1 { + max-width: 80rem; + margin: 0 auto; +} + +@media (max-width:25rem) { + .c0 { + padding: 0 0.5rem; + } +} + +@media (min-width:25rem) { + .c0 { + padding: 0 1rem; + } +} + +@media (min-width:62.9375rem) { + .c0 { + padding: 0 1rem; + } +} + +
+
+
+
+
, +

+ Holla +

, + .c1 { + background-color: #B80000; + height: 5rem; + width: 100%; +} + +.c2 { + max-width: 80rem; + margin: 0 auto; +} + +.c8 { + padding: 0.5rem 0 0.5rem; + color: #FFFFFF; + font-weight: 700; + -webkit-text-decoration: none; + text-decoration: none; + display: block; +} + +.c11 { + padding: 0.5rem 0 0.5rem; + color: #FFFFFF; + font-weight: 700; + -webkit-text-decoration: none; + text-decoration: none; + display: inline; +} + +.c7:hover .c9, +.c7:focus .c9 { + padding-bottom: 2px; + border-bottom: 2px solid #FFFFFF; +} + +.c5 { + border-bottom: 1px solid #3F3F42; + display: grid; + grid-auto-flow: column; + list-style-type: none; + margin: 0; + padding: 0 0 0.5rem; +} + +.c5 > li:first-child { + border-bottom: 1px solid #3F3F42; + padding: 0.5rem 0; + margin-bottom: 0.5rem; + grid-column: 1/-1; +} + +.c6 { + min-width: 50%; +} + +.c3 { + background-color: #222222; + font-size: 0.875rem; + line-height: 1rem; + font-family: ReithSans,Helvetica,Arial,sans-serif; +} + +.c4 { + max-width: 80rem; + margin: 0 auto; +} + +.c10 { + color: #FFFFFF; + margin: 0; + padding: 1rem 0; +} + +.c0 { + padding-top: 2rem; +} + +@media (max-width:25rem) { + .c1 { + padding: 0 0.5rem; + } +} + +@media (min-width:25rem) { + .c1 { + padding: 0 1rem; + } +} + +@media (min-width:62.9375rem) { + .c1 { + padding: 0 1rem; + } +} + +@media (max-width:37.4375rem) { + .c5 { + grid-column-gap: 0.5rem; + grid-template-columns: repeat(2,1fr); + grid-template-rows: repeat(4,auto); + } +} + +@media (min-width:37.5rem) and (max-width:62.9375rem) { + .c5 { + grid-column-gap: 1rem; + grid-template-columns: repeat(3,1fr); + grid-template-rows: repeat(3,auto); + } +} + +@media (min-width:63rem) and (max-width:80rem) { + .c5 { + grid-column-gap: 1rem; + grid-template-columns: repeat(4,1fr); + grid-template-rows: repeat(3,auto); + } +} + +@media (min-width:80rem) { + .c5 { + grid-column-gap: 1rem; + grid-template-columns: repeat(5,1fr); + grid-template-rows: repeat(3,auto); + } +} + +@supports not (display:grid) { + .c5 > li:first-child { + width: 100%; + } +} + +@supports not (display:grid) { + .c6 { + display: inline-block; + } +} + +@media (max-width:25rem) { + .c3 { + padding: 0 0.5rem; + } +} + +@media (min-width:25rem) { + .c3 { + padding: 0 1rem; + } +} + +@media (min-width:20rem) and (max-width:37.4375rem) { + .c3 { + line-height: 1.125rem; + } +} + +@media (min-width:37.5rem) { + .c3 { + font-size: 0.8125rem; + } +} + +, +] +`; diff --git a/src/app/containers/PageHandlers/withData.jsx b/src/app/containers/PageHandlers/withData.jsx new file mode 100644 index 00000000000..db69965c9f9 --- /dev/null +++ b/src/app/containers/PageHandlers/withData.jsx @@ -0,0 +1,54 @@ +import React from 'react'; +import { shape, element } from 'prop-types'; +import articlePropTypes from '../../models/propTypes/article'; +import ErrorMain from '../ErrorMain'; +import nodeLogger from '../../helpers/logger.node'; +import deepGet from '../../helpers/json/deepGet'; + +const logger = nodeLogger(__filename); + +// checks for data and status keys, setting default status if not found +const constructRenderObject = data => ({ + status: deepGet(['status'], data) || 500, + pageData: deepGet(['pageData'], data), +}); + +// checks for pageData and 200 status +const shouldRender = data => { + const { status, pageData } = constructRenderObject(data); + + const hasDataAnd200Status = pageData && status === 200; + + return { hasDataAnd200Status, status, pageData }; +}; + +const WithData = Component => { + const DataContainer = ({ data, ...props }) => { + const { hasDataAnd200Status, status, pageData } = shouldRender(data); + if (hasDataAnd200Status) { + try { + return ; + } catch (err) { + logger.error(err); + } + } + + return ; + }; + + DataContainer.propTypes = { + data: shape(articlePropTypes), + }; + + DataContainer.defaultProps = { + data: null, + }; + + return DataContainer; +}; + +WithData.propTypes = { + Component: element, +}; + +export default WithData; diff --git a/src/app/containers/PageHandlers/withData.test.jsx b/src/app/containers/PageHandlers/withData.test.jsx new file mode 100644 index 00000000000..e3fca2ce34f --- /dev/null +++ b/src/app/containers/PageHandlers/withData.test.jsx @@ -0,0 +1,61 @@ +import React from 'react'; +import { shouldShallowMatchSnapshot } from '../../helpers/tests/testHelpers'; +import { articleDataNews } from '../Article/fixtureData'; +import WithData from './withData'; + +describe('withData HOC', () => { + const Component = () =>

Hola

; + const WithDataHOC = WithData(Component); + + const noDataProps = { + data: null, + }; + + const noAssetData = { + data: { + status: 200, + }, + }; + + const non200StatusProps = { + data: { + pageData: articleDataNews, + status: 157, + }, + }; + + const validProps = { + data: { + pageData: articleDataNews, + status: 200, + }, + }; + + describe('with no data', () => { + shouldShallowMatchSnapshot( + `should return the errorMain component and 500 status`, + , + ); + }); + + describe('with missing articleData', () => { + shouldShallowMatchSnapshot( + `should return the errorMain component`, + , + ); + }); + + describe('with valid props', () => { + shouldShallowMatchSnapshot( + `should return the passed in component`, + , + ); + }); + + describe('with non 200 status', () => { + shouldShallowMatchSnapshot( + `should return the errorMain component`, + , + ); + }); +}); diff --git a/src/app/containers/PageHandlers/withError.jsx b/src/app/containers/PageHandlers/withError.jsx new file mode 100644 index 00000000000..869eaff0db4 --- /dev/null +++ b/src/app/containers/PageHandlers/withError.jsx @@ -0,0 +1,26 @@ +import React from 'react'; +import { string, element } from 'prop-types'; +import ErrorMain from '../ErrorMain'; + +const WithError = Component => { + const ErrorContainer = ({ error, ...props }) => { + if (!error) return ; + return ; + }; + + ErrorContainer.propTypes = { + error: string, + }; + + ErrorContainer.defaultProps = { + error: true, + }; + + return ErrorContainer; +}; + +WithError.propTypes = { + Component: element, +}; + +export default WithError; diff --git a/src/app/containers/PageHandlers/withError.test.jsx b/src/app/containers/PageHandlers/withError.test.jsx new file mode 100644 index 00000000000..321a6a063b2 --- /dev/null +++ b/src/app/containers/PageHandlers/withError.test.jsx @@ -0,0 +1,22 @@ +import React from 'react'; +import { shouldShallowMatchSnapshot } from '../../helpers/tests/testHelpers'; +import WithError from './withError'; + +describe('withError HOC', () => { + const Component = () =>

Hola

; + const ErrorHOC = WithError(Component); + + describe('with error', () => { + shouldShallowMatchSnapshot( + `should return the errorMain component`, + , + ); + }); + + describe('with no error', () => { + shouldShallowMatchSnapshot( + `should return the passed in component`, + , + ); + }); +}); diff --git a/src/app/containers/PageHandlers/withLoading.jsx b/src/app/containers/PageHandlers/withLoading.jsx new file mode 100644 index 00000000000..e134ad1c97f --- /dev/null +++ b/src/app/containers/PageHandlers/withLoading.jsx @@ -0,0 +1,32 @@ +import React from 'react'; +import { bool, element } from 'prop-types'; +import { GhostWrapper, GridItemConstrainedMedium } from '../../lib/styledGrid'; + +const WithLoading = Component => { + const LoadingContainer = ({ loading, ...props }) => { + if (!loading) return ; + return ( +
+ + + +
+ ); + }; + + LoadingContainer.propTypes = { + loading: bool, + }; + + LoadingContainer.defaultProps = { + loading: false, + }; + + return LoadingContainer; +}; + +WithLoading.propTypes = { + Component: element, +}; + +export default WithLoading; diff --git a/src/app/containers/PageHandlers/withLoading.test.jsx b/src/app/containers/PageHandlers/withLoading.test.jsx new file mode 100644 index 00000000000..c54e6a2a994 --- /dev/null +++ b/src/app/containers/PageHandlers/withLoading.test.jsx @@ -0,0 +1,22 @@ +import React from 'react'; +import { shouldShallowMatchSnapshot } from '../../helpers/tests/testHelpers'; +import WithLoading from './withLoading'; + +describe('withLoading HOC', () => { + const Component = () =>

Hola

; + const LoadingHOC = WithLoading(Component); + + describe('and the loading prop set to true', () => { + shouldShallowMatchSnapshot( + `should return the loading component`, + , + ); + }); + + describe('and no loading prop', () => { + shouldShallowMatchSnapshot( + `should return the passed in component`, + , + ); + }); +}); diff --git a/src/app/containers/PageHandlers/withPageWrapper.jsx b/src/app/containers/PageHandlers/withPageWrapper.jsx new file mode 100644 index 00000000000..eb4a9e8651a --- /dev/null +++ b/src/app/containers/PageHandlers/withPageWrapper.jsx @@ -0,0 +1,26 @@ +import React from 'react'; +import { string, shape } from 'prop-types'; +import articlePropTypes from '../../models/propTypes/article'; +import PageWrapper from '../../Layouts/defaultPageWrapper'; + +const WithPageWrapper = Component => { + const PageWrapperContainer = props => ( + + + + ); + + PageWrapperContainer.propTypes = { + data: shape(articlePropTypes), + bbcOrigin: string, + }; + + PageWrapperContainer.defaultProps = { + data: null, + bbcOrigin: null, + }; + + return PageWrapperContainer; +}; + +export default WithPageWrapper; diff --git a/src/app/containers/PageHandlers/withPageWrapper.test.jsx b/src/app/containers/PageHandlers/withPageWrapper.test.jsx new file mode 100644 index 00000000000..97aef334852 --- /dev/null +++ b/src/app/containers/PageHandlers/withPageWrapper.test.jsx @@ -0,0 +1,17 @@ +import React from 'react'; +import { shouldMatchSnapshot } from '../../helpers/tests/testHelpers'; +import WithPageWrapper from './withPageWrapper'; + +const dataProps = { + isAmp: false, + service: 'news', +}; + +describe('with pageWarpper', () => { + const PageWrapperContainer = () =>

Holla

; + const PageWrapperHOC = WithPageWrapper(PageWrapperContainer); + shouldMatchSnapshot( + `should render correctly`, + , + ); +}); diff --git a/src/app/helpers/compose.js b/src/app/helpers/compose.js new file mode 100644 index 00000000000..d82149742bd --- /dev/null +++ b/src/app/helpers/compose.js @@ -0,0 +1,9 @@ +/* + * © Andrew Clark https://github.com/acdlite + * https://github.com/acdlite/recompose + */ + +const compose = (...funcs) => + funcs.reduce((a, b) => (...args) => a(b(...args)), arg => arg); + +export default compose; diff --git a/src/app/helpers/compose.test.js b/src/app/helpers/compose.test.js new file mode 100644 index 00000000000..1c497eeb3cd --- /dev/null +++ b/src/app/helpers/compose.test.js @@ -0,0 +1,47 @@ +/* + * © Andrew Clark https://github.com/acdlite + * https://github.com/acdlite/recompose + */ + +import compose from './compose'; + +test('compose composes from right to left', () => { + const double = x => x * 2; + const square = x => x * x; + expect(compose(square)(5)).toBe(25); + expect( + compose( + square, + double, + )(5), + ).toBe(100); + expect( + compose( + double, + square, + double, + )(5), + ).toBe(200); +}); + +test('compose can be seeded with multiple arguments', () => { + const square = x => x * x; + const add = (x, y) => x + y; + expect( + compose( + square, + add, + )(1, 2), + ).toBe(9); +}); + +test('compose returns the identity function if given no arguments', () => { + expect(compose()(1, 2)).toBe(1); + expect(compose()(3)).toBe(3); + expect(compose()()).toBe(undefined); +}); + +test('compose returns the first function if given only one', () => { + const fn = x => x * x; + expect(compose(fn)(3)).toBe(fn(3)); +});