-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core: normalize creation of NodeValue #11877
Changes from 44 commits
90018b2
f7e38f5
78f386a
32856e7
e2dd63d
f38bf41
ce9839b
4c903de
0413a4a
d4bd2bf
02dc215
ce2827a
c81a636
70d8210
df5e8dd
4e59476
147b98c
a4051a1
9eea206
d79b23f
b8c76ed
2088109
7b01e8c
4cc1eea
aae0a9f
77af88d
d35fd5e
eb897cf
9c10cda
d643baf
9638b90
c526772
b893bb1
606db62
8c98d05
ed13e97
3195324
af84552
f799583
f2b649b
d850391
7b7aeb3
2e37d0b
685d356
a04e261
098411b
cc8cdb3
f763a1f
25210c5
ba30ab5
0679ff5
68ee66f
d7c562c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -146,7 +146,7 @@ class Plugins extends Audit { | |
|
||
return { | ||
source: { | ||
type: /** @type {'node'} */ ('node'), | ||
...Audit.makeNodeValue(plugin.node), | ||
snippet: `<${tagName}${attributes}>${params}</${tagName}>`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this ends up rendering completely differently in the report (it was basically just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
examples? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe, if we wanted to we could parameterize There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
unlike the
yeah, though I guess do we care about the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ehhh let's just use the default snippet so we can keep this simple. it'll be more identifiable now with the devtools node path and everything else, and truncation will prevent the worse cases. |
||
}, | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,13 +248,8 @@ function targetToTableNode(target) { | |
const boundingRect = getBoundingRect(target.clientRects); | ||
|
||
return { | ||
type: 'node', | ||
lhId: target.node.lhId, | ||
snippet: target.node.snippet, | ||
path: target.node.devtoolsNodePath, | ||
selector: target.node.selector, | ||
...Audit.makeNodeValue(target.node), | ||
boundingRect, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which boundingRect should be used here? Using the boundingRect of all clientRects goes back to the first commit of #10716, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll followup with this in a different PR. |
||
nodeLabel: target.node.nodeLabel, | ||
}; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we add
explanation
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like this gets included in the report 🤔 . Is it supposed to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only purpose I can see is in the smoke tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#5402