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

Remove unnecessary example #591

Closed
wants to merge 1 commit into from
Closed

Conversation

lanker
Copy link
Contributor

@lanker lanker commented Jun 12, 2024

The two examples given only differed on how the div element was closed (self-closing vs non-self-closing), which is not relevant to the rule at hand. To avoid confusing the reader, let's keep only one of the examples.

The two examples given only differed on how the div element was closed
(self-closing vs non-self-closing), which is not relevant to the rule at
hand. To avoid confusing the reader, let's keep only one of the
examples.
Copy link

netlify bot commented Jun 12, 2024

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit ba6f63e
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/666946fb938f300008a86b02
😎 Deploy Preview https://deploy-preview-591--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Sec-ant
Copy link
Member

Sec-ant commented Jun 12, 2024

Hi @lanker

  • The page is generated from the source code. So to remove the example, you'll have to edit the document comments in the source.
  • I think both of the two examples exist because we want to make sure the rule works on both the openning tag and the self-closing element (We run our analyzer on the code blocks in this page when building it). But I agree this may confuse the reader and I also noticed we already have dedicated test cases for this purpose. So I think removing one of them is ok.
  • Given that the rule useSelfClosingElements exists, what do you think of removing the other example, simply for consistency?

@lanker
Copy link
Contributor Author

lanker commented Jun 12, 2024

Hi @lanker

Hi, thanks for the quick feedback!

* The page is generated from the source code. So to remove the example, you'll have to edit the [document comments in the source](https://github.com/biomejs/biome/blob/0d9b60a181cf59004815c928c277d13f60fb3b9b/crates/biome_js_analyze/src/lint/a11y/use_key_with_click_events.rs#L16-L19).

Ah! Sorry for not looking more carefully how it's done.

* I think both of the two examples exist because we want to make sure the rule works on both the openning tag and the self-closing element (We run our analyzer on the code blocks in this page when building it). But I agree this may confuse the reader and I also noticed we already have [dedicated test cases](https://github.com/biomejs/biome/blob/0d9b60a181cf59004815c928c277d13f60fb3b9b/crates/biome_js_analyze/tests/specs/a11y/useKeyWithClickEvents/invalid.jsx#L3-L4) for this purpose. So I think removing one of them is ok.

* Given that the rule [useSelfClosingElements](https://biomejs.dev/linter/rules/use-self-closing-elements/) exists, what do you think of removing the other example, simply for consistency?

Yep, makes sense to keep the self-closing example instead. I opened a new PR towards the biome repo: biomejs/biome#3185 and will close this one.

@lanker lanker closed this Jun 12, 2024
@lanker lanker deleted the lanker/onclick.docs branch June 12, 2024 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants