Skip to content
This repository has been archived by the owner on Dec 3, 2024. It is now read-only.

chore: eslint migration #186

Merged
merged 17 commits into from
May 26, 2020
Merged

chore: eslint migration #186

merged 17 commits into from
May 26, 2020

Conversation

Averethel
Copy link
Member

Migrate to new eslint rulest

@Averethel Averethel requested review from masone, lkappeler and a team May 19, 2020 11:48
tsconfig.json Show resolved Hide resolved
src/components/dropdown/menu.tsx Show resolved Hide resolved
@@ -72,7 +72,7 @@ const filterOptions = (noResults) => (allOptions, text) => {
return allOptions
}

const specialChars = /[-\/\\^$*+?.()|[\]{}]/g
const specialChars = /[-/\\^$*+?.()|[\]{}]/g
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really the same regex without the escaping backslash?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's an unnecessary escape character according to the linter

@@ -93,6 +93,7 @@ const Input = forwardRef<HTMLInputElement, Props>(
value={value || ""}
placeholder={placeholder || ""}
className={classNames("w-12/12", className, {
// eslint-disable-next-line @typescript-eslint/naming-convention
Copy link
Contributor

Choose a reason for hiding this comment

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

is the plugin not in the latest version or does the variable set not cover block modifiers in the BEM convention?

Copy link
Member Author

Choose a reason for hiding this comment

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

Our BEM convention is a mixture of camelCase and snake_case. By default, you cannot mix cases. What we could do is to write a custom regexp to support our BEM syntax...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh... that's a pity

(I don't have a valuable comment to make)

@@ -211,9 +213,11 @@ const Select = forwardRef<HTMLInputElement, Props<any>>(
<>
{"withAutosuggest" in rest && rest.withAutosuggest ? (
<DropdownWithAutosuggest
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Copy link
Contributor

Choose a reason for hiding this comment

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

Hard to tell without looking into the code, perhaps we can change the way we pass the props or add form the props

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not find a way to add a type variable to existing constructs like forwardRef. This causes the any above and by consequence any here.

@@ -7,14 +7,15 @@ import { wInfo } from "./utils"

import Dropdown from "../src/components/dropdown"

const options = () => object("options", [
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we need to update the code examples as the format command does not format code in comments or is it fine to have them in the old format and let one fix formatting when copying the example to the project? For me it's completely fine to keep them as is to avoid addional efforts just to update formatting in code examples.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that's a valid point. I did not look into that...

@Averethel Averethel merged commit 9efcdbb into master May 26, 2020
@Averethel Averethel deleted the eslint-migration branch May 26, 2020 13:00
@autoricardo-bot
Copy link
Collaborator

🎉 This PR is included in version 6.1.0-CAR-4260-extract-header-fe37d3f547d0575fc7be01e15ab50bd541f06639.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@autoricardo-bot
Copy link
Collaborator

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

lkappeler added a commit that referenced this pull request May 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

5 participants