Support XLink in the imperative API lint rule [devtools/devtools-frontend : main]
0 views
Skip to first unread message
Nikolay Vitkov (Gerrit)
4:40 AM (9 hours ago) 4:40 AM
to Danil Somsikov, Devtools-frontend LUCI CQ, AyeAye, devtools-rev...@chromium.org
Attention needed from Danil Somsikov
Nikolay Vitkov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Nikolay Vitkov
| Code-Review | +1 |
3 comments
Patchset-level comments
File scripts/eslint_rules/lib/no-imperative-dom-api/x-link.ts
Line 38, Patchset 5 (Latest):
key: 'jslog',Nikolay Vitkov . unresolved
Nit: I mimicked that we do for other components and adding the `jslogContext` directly as the `jslogContextString` should work correctly.
File scripts/eslint_rules/tests/no-imperative-dom-api.test.ts
Line 1421, Patchset 5 (Latest):
tabindex="0">Google</devtools-link>Nikolay Vitkov . unresolved
Can we check if the tabindex is 0 and not set it at all. That is handled by the component.
Related details
Attention is currently required from:
- Danil Somsikov
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
4:52 AM (9 hours ago) 4:52 AM
to Nikolay Vitkov, Devtools-frontend LUCI CQ, AyeAye, devtools-rev...@chromium.org
Danil Somsikov added 2 comments![]( "Open in Gerrit")
File scripts/eslint_rules/lib/no-imperative-dom-api/x-link.ts
Nit: I mimicked that we do for other components and adding the `jslogContext` directly as the `jslogContextString` should work correctly.
Danil Somsikov
Done
File scripts/eslint_rules/tests/no-imperative-dom-api.test.ts
Can we check if the tabindex is 0 and not set it at all. That is handled by the component.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
4:52 AM (9 hours ago) 4:52 AM
to Nikolay Vitkov, Devtools-frontend LUCI CQ, AyeAye, devtools-rev...@chromium.org
Danil Somsikov voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devtools-frontend LUCI CQ (Gerrit)
5:29 AM (8 hours ago) 5:29 AM
to Danil Somsikov, Nikolay Vitkov, AyeAye, devtools-rev...@chromium.org
Devtools-frontend LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: scripts/eslint_rules/lib/no-imperative-dom-api/x-link.ts
Insertions: 7, Deletions: 6.
@@ -33,13 +33,14 @@
domFragment.classList.push(className);
}
const jslogContext = node.arguments[4];
- const jslogContextString = jslogContext ? sourceCode.getText(jslogContext) : '';
- domFragment.attributes.push({
- key: 'jslog',
- value: `\${VisualLogging.link(${jslogContextString}).track({click: true, keydown: 'Enter|Space'})}`
- });
+ if (jslogContext && !isIdentifier(jslogContext, 'undefined')) {
+ domFragment.bindings.push({
+ key: 'jslogContext',
+ value: jslogContext,
+ });
+ }
const tabIndex = node.arguments[5];
- if (tabIndex) {
+ if (tabIndex && (tabIndex.type !== 'Literal' || tabIndex.value !== 0)) {
domFragment.attributes.push({
key: 'tabindex',
value: tabIndex,
```
```
The name of the file: scripts/eslint_rules/tests/no-imperative-dom-api.test.ts
Insertions: 3, Deletions: 6.
@@ -1408,7 +1408,7 @@
this.contentElement.appendChild(
UI.XLink.XLink.create('https://google.com', 'Google', 'some-class', undefined, 'some-context', 0));
this.contentElement.appendChild(
- UI.XLink.XLink.create('https://chromium.org', 'Chromium'));
+ UI.XLink.XLink.create('https://chromium.org', 'Chromium', undefined, undefined, undefined, 1));
}
}`,
output: `
@@ -1416,11 +1416,8 @@
export const DEFAULT_VIEW = (input, _output, target) => {
render(html\`
<div>
- <devtools-link class="some-class" href="https://google.com"
- jslog=\${VisualLogging.link('some-context').track({click: true, keydown: 'Enter|Space'})}
- tabindex="0">Google</devtools-link>
- <devtools-link href="https://chromium.org"
- jslog=\${VisualLogging.link().track({click: true, keydown: 'Enter|Space'})}>Chromium</devtools-link>
+ <devtools-link class="some-class" href="https://google.com" .jslogContext=\${'some-context'}>Google</devtools-link>
+ <devtools-link href="https://chromium.org" tabindex="1">Chromium</devtools-link>
</div>\`,
target, {host: input});
};
```
Change information
Commit message:
Support XLink in the imperative API lint rule
Bug: 407749531
Change-Id: Ibef4be3d07e3ed16d9ee6a92e8b99f8379f412c6
Reviewed-by: Nikolay Vitkov <nvi...@chromium.org>
Commit-Queue: Danil Somsikov <d...@chromium.org>
Auto-Submit: Danil Somsikov <d...@chromium.org>
Files:
- M scripts/eslint_rules/lib/no-imperative-dom-api.ts
- A scripts/eslint_rules/lib/no-imperative-dom-api/x-link.ts
- M scripts/eslint_rules/tests/no-imperative-dom-api.test.ts
Change size: M
Delta: 3 files changed, 85 insertions(+), 0 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Nikolay Vitkov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages