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

Add new rule: no-restricted-html-elements #1820

Merged
merged 21 commits into from Mar 23, 2022

Conversation

doug-wade
Copy link
Contributor

Hi there! I would like to add a new rule to the plugin inspired by the React forbid-elements rule. I'm trying to solve a problem with a large project we're working on at work where developers aren't using our Button component from our component library, but are instead are using the button html element. I would like to forbid use of the button html tag altogether to encourage adoption, so I would like to add:

{
  "vue/html-forbid-elements": ["error", { "forbid": ["button"] }]
}

to our eslintrc file and then emit linting errors when new button tags are added. I will likely also use this for anchor tags to encourage use of our Link component.

Another use case might be to restrict use of deprecated html elements, like marquee.

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

Thank you for this PR.

I think the rule name should be no-restricted-html-elements, just like any other user-specified disallow rule.
e.g.

Could you change the rule name?

Also, like any other no-restricted rules, could you change the options to be accepted in an array, and allow users to specify custom messages in the options using object forms?

Perhaps the source code for the following rule will be helpful to you.
https://eslint.vuejs.org/rules/no-restricted-block.html

docs/rules/html-forbid-elements.md Outdated Show resolved Hide resolved
docs/rules/html-forbid-elements.md Outdated Show resolved Hide resolved
docs/rules/html-forbid-elements.md Outdated Show resolved Hide resolved
tests/lib/rules/html-forbid-elements.js Outdated Show resolved Hide resolved
@doug-wade
Copy link
Contributor Author

@ota-meshi Thank you so much for leaving a review so quickly! It's my first time contributing to this repo, so it was rough around the edges, and I appreciate your feedback.

@doug-wade doug-wade changed the title Add new rule: html-forbid-elements Add new rule: no-restricted-html-elements Mar 20, 2022
Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

I only suggest some minor spelling/wording and code style improvements. The rest looks fine though from my side. Thanks!

lib/rules/no-restricted-html-elements.js Outdated Show resolved Hide resolved
docs/rules/no-restricted-html-elements.md Outdated Show resolved Hide resolved
docs/rules/no-restricted-html-elements.md Outdated Show resolved Hide resolved
docs/rules/no-restricted-html-elements.md Outdated Show resolved Hide resolved
lib/rules/no-restricted-html-elements.js Outdated Show resolved Hide resolved
docs/rules/no-restricted-html-elements.md Outdated Show resolved Hide resolved
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

Almost LGTM. I have some simple change requests in usage.

tests/lib/rules/no-restricted-html-elements.js Outdated Show resolved Hide resolved
tests/lib/rules/no-restricted-html-elements.js Outdated Show resolved Hide resolved
tests/lib/rules/no-restricted-html-elements.js Outdated Show resolved Hide resolved
tests/lib/rules/no-restricted-html-elements.js Outdated Show resolved Hide resolved
tests/lib/rules/no-restricted-html-elements.js Outdated Show resolved Hide resolved
tests/lib/rules/no-restricted-html-elements.js Outdated Show resolved Hide resolved
tests/lib/rules/no-restricted-html-elements.js Outdated Show resolved Hide resolved
doug-wade and others added 16 commits March 22, 2022 19:40
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Co-authored-by: Yosuke Ota <otameshiyo23@gmail.com>
Co-authored-by: Yosuke Ota <otameshiyo23@gmail.com>
Co-authored-by: Yosuke Ota <otameshiyo23@gmail.com>
Co-authored-by: Yosuke Ota <otameshiyo23@gmail.com>
Co-authored-by: Yosuke Ota <otameshiyo23@gmail.com>
Co-authored-by: Yosuke Ota <otameshiyo23@gmail.com>
Co-authored-by: Yosuke Ota <otameshiyo23@gmail.com>
Co-authored-by: Yosuke Ota <otameshiyo23@gmail.com>
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

Thanks for implementing the feedback! :)

@ota-meshi ota-meshi merged commit 1ce68fa into vuejs:master Mar 23, 2022
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

3 participants