Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyze): useIframeTitle #4067

Merged
merged 7 commits into from
Dec 21, 2022
Merged

feat(rome_js_analyze): useIframeTitle #4067

merged 7 commits into from
Dec 21, 2022

Conversation

unvalley
Copy link
Contributor

@unvalley unvalley commented Dec 17, 2022

Summary

Closes #3945

Test Plan

cargo test -p rome_js_analyze -- use_iframe_title

Documentation

  • The PR requires documentation
    • and I executed cargo lintdoc
  • [ ] I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Dec 17, 2022

Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit ee0d14b
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/63a2896a3429fb0009582038
😎 Deploy Preview https://deploy-preview-4067--docs-rometools.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 settings.

@unvalley unvalley marked this pull request as draft December 17, 2022 14:58
@unvalley unvalley marked this pull request as ready for review December 17, 2022 16:53
@unvalley unvalley requested review from xunilrj and removed request for ematipico and leops December 19, 2022 14:33
Some(
RuleDiagnostic::new(
rule_category!(),
state.node.syntax().text_trimmed_range(),
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like state.node is always set to the query node, in this case you don't need to clone it into the state and can simply retrieve it in the diagnostic method by calling ctx.query() (then you can just use () as the signal type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've done at 8100a2c.

Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Once we address the feedback, I think we can merge the PR


3 │ <iframe />
4 │ <iframe></iframe>
> 5 │ <iframe {...props} />
Copy link
Contributor

Choose a reason for hiding this comment

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

This case should not trigger the rule. props might contain title, we don't know that.

Copy link
Contributor Author

@unvalley unvalley Dec 20, 2022

Choose a reason for hiding this comment

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

Thanks, I've fixed it at 8100a2c.
imho, this behavior will be different from ESLint.
jsx-eslint/eslint-plugin-jsx-a11y@main/docs/rules/iframe-has-title.md

author unvalley <kirohi.code@gmail.com> 1671288906 +0900
committer unvalley <kirohi.code@gmail.com> 1671554652 +0900

feat(rome_js_analyze): useIframeTitle

docs: website

chore: delete waste file

refactor: remove waste node declaration

chore: add configurations

refactor: remove missing_prop and edit message
docs: update useIframeTitle.md

chore: codegen

chore: cargo fmt
chore: cargo fmt

chore: codegen stuff

chore: cargo fmt
@ematipico ematipico merged commit bd4c3e4 into rome:main Dec 21, 2022
@unvalley unvalley deleted the feat/use-iframe-title branch December 21, 2022 09:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useIframeTitle
4 participants