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

Improve testing environment for whole repo #11

Merged
merged 1 commit into from
Feb 22, 2018
Merged

Conversation

jakubzaba
Copy link
Contributor

Add circleci
Move enzyme into root package.json
Add testing scripts into root package.json
Turn off some eslint rules

.eslintrc Outdated
],
"rules": {
"prettier/prettier": "error",
"prettier/prettier": [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, use .prettierrc. also, the only rules that make sense configuring are the ones already present

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted

.eslintrc Outdated
"trailingComma": "all"
}
],
"import/no-extraneous-dependencies": "off",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @oreqizer . If this is causing issues in tests then use a glob pattern to fix it. https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-extraneous-dependencies.md

Copy link
Contributor Author

@jakubzaba jakubzaba Feb 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not working for yarn workspaces. Globs didn't help for all cases.
import-js/eslint-plugin-import#458 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so turn off where needed

.eslintrc Outdated
}
],
"import/no-extraneous-dependencies": "off",
"react/jsx-filename-extension": "off",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree with turning this off, I don't see any reason to make difference whether file has some JSX or not, it's like prefixing all string variables with s...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to .js only. I don't see benefit to use .jsx.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well JSX is not a valid JS syntax, that's a pretty simple reason. whatever, you guys are the majority

.eslintrc Outdated
],
"import/no-extraneous-dependencies": "off",
"react/jsx-filename-extension": "off",
"react/require-default-props": "off",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

.eslintrc Outdated
"import/no-extraneous-dependencies": "off",
"react/jsx-filename-extension": "off",
"react/require-default-props": "off",
"jsx-a11y/label-has-for": "off",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment. Label is wrapping input, no need for for.

@jakubzaba
Copy link
Contributor Author

@oreqizer pls take a look on changes.

@jakubzaba
Copy link
Contributor Author

To push this forward I suggest to merge it. All comments were fixed or answered.
@fallion @oreqizer

.eslintrc Outdated
],
"rules": {
"prettier/prettier": "error",
"import/no-extraneous-dependencies": "off",
Copy link
Contributor

@oreqizer oreqizer Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, see comment in the original thread (or here lol):

turn off where needed. this rule is perfectly legit

.eslintrc Outdated
}
],
"import/no-extraneous-dependencies": "off",
"react/jsx-filename-extension": "off",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well JSX is not a valid JS syntax, that's a pretty simple reason. whatever, you guys are the majority

@oreqizer
Copy link
Contributor

sorry for the delay, guys. yesterday was hectic

@jakubzaba
Copy link
Contributor Author

jakubzaba commented Feb 22, 2018

I added devDependencies for import/no-extraneous-dependencies and react/require-default-props because workaround for Flow would be insane and we want undefined optional props.

Add circleci
Move enzyme into root package.json
Add testing scripts into root package.json
Turn off some eslint rules
@oreqizer
Copy link
Contributor

The point about working around Flow is invalid IMO, but OK, requiring all props to be defaulted would be daunting I guess

@jakubzaba jakubzaba merged commit a1eda68 into master Feb 22, 2018
@fallion fallion deleted the testing-env branch February 22, 2018 12:28
tomashapl pushed a commit that referenced this pull request Sep 11, 2019
vepor pushed a commit that referenced this pull request Aug 27, 2020
vepor pushed a commit that referenced this pull request Nov 4, 2020
bul-nick-al pushed a commit to bul-nick-al/orbit that referenced this pull request Aug 19, 2022
Update List and ListItem components to support TextLinks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants