added _app.js and _document.js and set up getInitialProps #106
Conversation
|
Can you split this to multiple PRs for multiple features? It will make it easier to review and bring in changes incrementally |
I can move the _app.js into a separate commit, but all the rest of the changes are associated with either adding _document.js or making the global header a title only. Either way those changes go together. What would you prefer I do? |
|
Please follow the commit message expectations - (npm install should be including husky to enforce this on commit). For details, check the contribution guidelines |
|
@amclin sorry I didn't see that before, I will adjust accordingly. |
KevinDanikowski
force-pushed
the
feat/getInitialProps
branch
from
88cc035
to
6d013e3
Compare
| @@ -89,6 +89,7 @@ | |||
| ], | |||
| "coveragePathIgnorePatterns": [ | |||
| "<rootDir>/build/", | |||
| "<rootDir>/default/src/pages/_document.jsx", | |||
There was a problem hiding this comment.
removed since it is basic default code with only changes to head, but items in the head are tested in separate component tests.
There was a problem hiding this comment.
It shouldn't be ignored from code coverage
| [ | ||
| "next/babel", | ||
| { | ||
| "preset-env": { "targets": { "node": "current" } } |
There was a problem hiding this comment.
this was required to be able to extend a class in document.js
| import PropTypes from 'prop-types' | ||
| import Head from 'next/head' | ||
|
|
||
| const PageTitle = ({ title }) => { |
There was a problem hiding this comment.
change to title since that is the one thing that we may want to change from components outside document.js
There was a problem hiding this comment.
Is the key really necessary? I don't think eslint-react nor react is going to issue a warning here since it's wrapped in the element
|
To note, I have made the requested changes. |
| "plugins": ["react"], | ||
| "rules": { | ||
| "react/prop-types": ["error"], | ||
| "react/jsx-props-no-spreading": ["warn"], |
There was a problem hiding this comment.
Can you revert these rule changes and use comments to exclude single lines please?
| @@ -89,6 +89,7 @@ | |||
| ], | |||
| "coveragePathIgnorePatterns": [ | |||
| "<rootDir>/build/", | |||
| "<rootDir>/default/src/pages/_document.jsx", | |||
There was a problem hiding this comment.
It shouldn't be ignored from code coverage
| @@ -62,6 +62,7 @@ | |||
| "collectCoverageFrom": ["**/*.{js,jsx,ts,tsx}"], | |||
| "coveragePathIgnorePatterns": [ | |||
| "<rootDir>/build/", | |||
| "<rootDir>/src/pages/_document.jsx", | |||
There was a problem hiding this comment.
Shouldn't be ignored - not a good example to set. If you look at the client-internal repo forked from this, I believe I have an example of testing the _app.jsx or _document.jsx
| [ | ||
| "next/babel", | ||
| { | ||
| "preset-env": { "targets": { "node": "current" } } |
| @@ -19,6 +19,9 @@ | |||
| }, | |||
| "plugins": ["react"], | |||
| "rules": { | |||
| "react/prop-types": ["error"] | |||
| "react/prop-types": ["error"], | |||
There was a problem hiding this comment.
Please revert these and use comments for single-line excludes
| import PropTypes from 'prop-types' | ||
| import Head from 'next/head' | ||
|
|
||
| const PageTitle = ({ title }) => { |
There was a problem hiding this comment.
Is the key really necessary? I don't think eslint-react nor react is going to issue a warning here since it's wrapped in the element
| */ | ||
| import React from 'react' | ||
| import Document, { Html, Head, Main, NextScript } from 'next/document' | ||
| import Flavicon from '../components/molecules/Favicon' |
|
see PR #166 |
_app.js, _document.js, and getInitialProps have been set up.
Notes have been added to each of those items about what to put in them.
Further, the global header has been changed to PageTitle to be more representative of what it is. The items origionally in global header were moved to _document.js
Fixes #127
Fixes #3