-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Update sewing-kit #532
Update sewing-kit #532
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,15 @@ | ||
/* eslint-disable no-console */ | ||
|
||
import {execSync} from 'child_process'; | ||
import {ensureDirSync, writeFileSync, readFileSync} from 'fs-extra'; | ||
import {join, resolve as resolvePath} from 'path'; | ||
import {ensureDirSync, writeFileSync, readFileSync} from 'fs-extra'; | ||
import {rollup} from 'rollup'; | ||
import {cp, mv, rm} from 'shelljs'; | ||
import copyfiles from 'copyfiles'; | ||
|
||
import createRollupConfig from '../config/rollup'; | ||
import generateSassBuild from './sass-build'; | ||
import packageJSON from '../package.json'; | ||
import generateSassBuild from './sass-build'; | ||
|
||
const root = resolvePath(__dirname, '..'); | ||
const build = resolvePath(root, 'build'); | ||
|
@@ -38,9 +38,6 @@ rm('-rf', resolvePath(root, 'types/src')); | |
|
||
mv(resolvePath(intermediateBuild, 'src/*'), intermediateBuild); | ||
|
||
const srcReadme = resolvePath(root, './src/components/README.md'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These variables were never used |
||
const destinationReadme = resolvePath(docs, './components/README.md'); | ||
|
||
copy(['./src/**/*.md', docs], {up: 1}).catch((error) => { | ||
console.error(error); | ||
process.exit(1); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
/* eslint-disable no-console */ | ||
const puppeteer = require('puppeteer'); | ||
const pa11y = require('pa11y'); | ||
const fs = require('fs'); | ||
const shitlistCheck = require('./pa11y-utilities.js').shitlistCheck; | ||
|
||
const shitlist = require('./../a11y_shitlist.json'); | ||
|
@@ -62,7 +61,7 @@ async function runPa11y() { | |
), | ||
); | ||
|
||
urls.map((path) => { | ||
urls.forEach((path) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This stops eslint complaining that there is no return value as in this case we don't care about the return value |
||
const currentBrowser = browsers[browserIndex % NUMBER_OF_BROWSERS]; | ||
browserIndex++; | ||
currentBrowser.taken = currentBrowser.taken.then(async () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,7 +67,6 @@ export function withAppProvider<OwnProps>() { | |
| React.ComponentClass<OwnProps & WithAppProviderProps> & C | ||
| React.SFC<OwnProps & WithAppProviderProps> & C, | ||
): React.ComponentClass<OwnProps> & C { | ||
// eslint-disable-next-line react/prefer-stateless-function | ||
class WithProvider extends React.Component<OwnProps, never> { | ||
static contextTypes = WrappedComponent.contextTypes | ||
? merge(WrappedComponent.contextTypes, polarisAppProviderContextTypes) | ||
|
@@ -139,6 +138,7 @@ export function withSticky() { | |
| React.ComponentClass<OwnProps & WithAppProviderProps> & C | ||
| React.SFC<OwnProps & WithAppProviderProps> & C, | ||
): any & C { | ||
// eslint-disable-next-line shopify/react-initialize-state | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not really sure what the correct fix is here so I hand waved it away as this is deep dark typescript magic that I didn't want to mess with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is an issue with the order of the generics here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm scared, and @AndrewMusgrave was poking around this area in https://github.com/Shopify/polaris-react-deprecated/pull/2274. I think that PR might solve this, and then because we've got the eslint-comments plugin it'll remind us to delete the ignore line if it is not needed |
||
class WithStickyManager extends React.Component< | ||
{}, | ||
OwnProps & WithAppProviderProps | ||
|
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.
Initially disabled rules!
import/no-cycle
shall be enabled in a follow-up PR as this one is already noisy enough as-is, and fixing it will require a lot of changes and there's potential for weirdness as I unpick the cycles.import/no-named-as-default
I'm not really sure what to do with. This currently has lots of errors and fixing it may require rethinking how we we write imports/exports. It doesn't help that I think the rule itself might be a bit buggy.shopify/jsx-no-complex-expressions
we probably want to keep disabled, This has 10 violations currently and in those cases we can't avoid it without some small rewrites that are more complex than I am comfortable putting in this PR that is mostly brainless changes.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.
All sounds good 👍 I think we might wanna create follow up issues for all of these so we don't forget about them.