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

Adds jsx-no-ampersands #1

Merged
merged 3 commits into from Jul 6, 2021
Merged

Conversation

niculistana
Copy link
Contributor

@niculistana niculistana commented May 26, 2021

Purpose

Adds lint rule check JSXExpressionContainer for ampersands and report a message.

Testing locally

  1. Installing
npm install --save -D ~/path/to/parent/eslint-plugin-grailed

or add the following to your package.json and run npm install

"devDependencies": {
    "eslint-plugin-grailed": "file:~/path/to/parent/eslint-plugin-grailed"
}
  1. Modify .eslintrc
{
  "plugins": [
    "grailed"
  ],
  "rules": {
    "grailed/jsx-no-ampersands": "warn"
  }
}
const TestComponent = () => {
     return <div>{true && <p>Show me the warning</p>}</div>
}

Links

Demo

https://www.loom.com/share/66e0fa36825249df96be81f90ac734bb

NOTE In the demo we install our package locally, but when we decide to use this in our production code base we will reference the package to our Github repo directly.

Adds minimal setup for custom eslint rule.

- Introduces a lint rule `jsx-no-ampersands` copied from on `jsx-embed-condition`: https://github.com/yannickcr/eslint-plugin-react/blob/ee232bbaef4fb04740413d5048877559abf06222/lib/rules/jsx-embed-condition.js
- TODO: add tests for `jsx-no-ampersands`
- To use this rule, add the following to `.eslintrc`:
```json
 {
  "plugins": [
   "grailed"
  ],
  "rules": {
   "grailed/jsx-no-ampersands": "warn"
  }
 }
```

Co-authored-by: Bean <10777333+GeneTheBean@users.noreply.github.com>
@niculistana niculistana added the Work In Progress PR is not ready to be merged because it is still being worked on. label May 26, 2021
@niculistana niculistana removed the Work In Progress PR is not ready to be merged because it is still being worked on. label Jun 17, 2021
@niculistana niculistana marked this pull request as ready for review June 17, 2021 21:18
@GeneTheBean GeneTheBean changed the title Adds jsx-no-ampersands (WIP) Adds jsx-no-ampersands Jun 24, 2021
@GeneTheBean GeneTheBean force-pushed the nlistana/add-jsx-no-ampersands branch from 280fc7f to a07ad38 Compare June 24, 2021 16:15
/**
* @fileoverview Prevents usage of && condition in JSX Embeds.
* @author Anees Iqbal <i@steelbrain.me>
* @see: https://github.com/yannickcr/eslint-plugin-react/blob/ee232bbaef4fb04740413d5048877559abf06222/lib/rules/jsx-embed-condition.js

Choose a reason for hiding this comment

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

OOO cool, it looks like adding jsx-embed-condition to eslint-plugin-react is a draft pull request, so we're making it happen earlier than they add it? Is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

From the discussion being had on their PR it doesn't seem like they are moving forward with it, so we're adding it ourselves for or own use case

@brianabaker
Copy link

I tried to test this through
npm install --save -D ~/path/to/parent/eslint-plugin-grailed
and got
Could not install from "../../path/to/parent/eslint-plugin-grailed" as it does not contain a package.json file

@GeneTheBean
Copy link
Contributor

I tried to test this through
npm install --save -D ~/path/to/parent/eslint-plugin-grailed
and got
Could not install from "../../path/to/parent/eslint-plugin-grailed" as it does not contain a package.json file

You have to clone this repo in another directory, and then from your grailed project you npm install referencing that directory

@brianabaker
Copy link

You have to clone this repo in another directory, and then from your grailed project you npm install referencing that directory

Ah ofc ty! I'll do that in a bit :)

@GeneTheBean GeneTheBean merged commit bfe5321 into main Jul 6, 2021
This rule disallows use of && inside JSX Embeds to avoid conditional numbers from being rendered.

## Why?
Here is an [acrticle](https://kentcdodds.com/blog/use-ternaries-rather-than-and-and-in-jsx) explaining the problems that can happen when you use && to conditionally render content in JSX.
Copy link

Choose a reason for hiding this comment

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

Typo!

@GeneTheBean GeneTheBean deleted the nlistana/add-jsx-no-ampersands branch July 26, 2021 22:49
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.

None yet

4 participants