-
Notifications
You must be signed in to change notification settings - Fork 227
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(infrastructure): react router screenshots #232
Conversation
Codecov Report
@@ Coverage Diff @@
## master #232 +/- ##
=======================================
Coverage 98.86% 98.86%
=======================================
Files 23 23
Lines 969 969
Branches 98 98
=======================================
Hits 958 958
Misses 11 11
Continue to review full report at Codecov.
|
"top-app-bar/shortCollapsed": "3565597194402f65bfc48edcea298a49c890c70100befe3ae19dd3cf24f17716", | ||
"top-app-bar/standard": "90534d59d40f8c050aae022e94b0afa022a00d1e00c19e3f92dcc1908efcd831", | ||
"top-app-bar/standardNoActionItems": "9102ece0efc0a040e0dc2b097c66d2ee5ecc9c91759aeb16ea5aa2baabe7307a", | ||
"top-app-bar/standardWithNavigationIconElement": "91b10df9c4faf85ba63c84b1cf82a1c1143a3d8c4ba7ed8af477fabdc65ec25e" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is shorter because chips and material-icon/menu are not in the list. Material-icon/menu is an old test which was removed. Chips not in yet.
@@ -1,7 +1,7 @@ | |||
import React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import classnames from 'classnames'; | |||
import {MDCSelectFoundation} from '@material/select'; | |||
import {MDCSelectFoundation} from '@material/select/dist/mdc.select'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is weird, why is this in this PR
scripts/webpack/component-globber.js
Outdated
@@ -0,0 +1,32 @@ | |||
const {lstatSync, readdirSync} = require('fs'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to just scripts
call it screenshot-directory-reader
and have one public function: read()
which returns an array of strings, where each string is a path under test/screenshot
scripts/webpack/index.js
Outdated
const {getComponents} = require('./component-globber'); | ||
const {getMaterialDependencies} = require('./material-dependencies'); | ||
|
||
function getMaterialExternals() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to just scripts
rename to package-name-converter
with one method: convertToImportPaths(packageNames)
which outputs an array of strings. Each string is an import path, e.g. "@material/button/dist/mdc.button". packageNames
is also an array of strings, but each string is the name of the package, e.g. "@material/button".
Then do the crazy dictionary objection creation stuff in the webpack config
@@ -0,0 +1,21 @@ | |||
const fs = require('fs'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to scripts
rename to package-json-reader
with one method: read
which outputs an array of strings, where each string is a package name, e.g. @material/button
.
test/screenshot/App.js
Outdated
const Home = () => { | ||
return ( | ||
<div> | ||
{COMPONENTS.sort().map((componentPath, index) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leave a comment pointing to webpack config, since this is a global
fixes #96