[Report-a-scam] Hook up screenshot to dialog 11/X [chromium/src : main]
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 9 comments![]( "Open in Gerrit")
.hidden {
display: none;
}Use cr_hidden_style.css instead, see examples https://source.chromium.org/search?q=cr_hidden_style.css.js%20-file:out%2F%20-file:go%2F%20-file:luci%2F%20-file:third_party%2F%20-file:appengine%2F%20-file:v8%2F&ss=chromium
.screenshot-placeholder {Since the DOM node with this CSS class already has an ID, no need to give it a class as well.
```suggestion
#screenshot-placeholder {
```
.screenshot-image {Same here.
```suggestion
#screenshot-image {
```
<img class="${this.includeScreenshot_ ? 'screenshot-image' : 'hidden'}"Use conditional rendering instead. See related comment further below.
id="screenshot-image" />"/>" is not proper HTML syntax.
```suggestion
id="screenshot-image">
```
See details at https://developer.mozilla.org/en-US/docs/Glossary/Void_element#self-closing_tags
<div
class="${this.includeScreenshot_ ? 'hidden' : 'screenshot-placeholder'}"
id="screenshot-placeholder">
<cr-icon icon="report_unsafe_site:visibility-off"></cr-icon>
</div>How about using conditional rendering instead? It seems much simpler thatn adding/removing a CSS class that shows/hides the element.
```suggestion
${!this.includeScreenshot_ ? html`
<div id="screenshot-placeholder">
<cr-icon icon="report_unsafe_site:visibility-off"></cr-icon>
</div>
` : ''}
```
@change="${this.onIncludeScreenshotChange_}">Incorrect indentation.
```suggestion
@change="${this.onIncludeScreenshotChange_}">
```
includeScreenshotCheckbox: CrCheckboxElement,Leverage this in tests to refer to the element, instead of using querySelector. Otherwise what is the point of adding this?
await whenCheck(checkbox, () => checkbox.checked);Why is this waiting necessary? Can you simply use `await microtasksFinished()` here? What triggers the checkbox to be checked?
Related details
- Peter Kotwicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 1 comment![]( "Open in Gerrit")
await whenCheck(checkbox, () => checkbox.checked);Why is this waiting necessary? Can you simply use `await microtasksFinished()` here? What triggers the checkbox to be checked?
FWIW, if this is triggered from the backend response callback, it is usually better to explicitly wait for the backend call to happen with something like
```
await this.handler.whenCalled(...);
await microtasksFinished();
```
than magically waiting for the state to change.
Related details
- Peter Kotwicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Kotwicz (Gerrit)
Peter Kotwicz added 9 comments![]( "Open in Gerrit")
Use cr_hidden_style.css instead, see examples https://source.chromium.org/search?q=cr_hidden_style.css.js%20-file:out%2F%20-file:go%2F%20-file:luci%2F%20-file:third_party%2F%20-file:appengine%2F%20-file:v8%2F&ss=chromium
Removed CSS rule because I switched to the conditional HTML notation you suggested.
Since the DOM node with this CSS class already has an ID, no need to give it a class as well.
```suggestion
#screenshot-placeholder {
```
Done
.screenshot-image {Peter KotwiczSame here.
```suggestion
#screenshot-image {
```
Done
<img class="${this.includeScreenshot_ ? 'screenshot-image' : 'hidden'}"Use conditional rendering instead. See related comment further below.
Done
"/>" is not proper HTML syntax.
```suggestion
id="screenshot-image">
```See details at https://developer.mozilla.org/en-US/docs/Glossary/Void_element#self-closing_tags
Thanks for explaining. I learned something!
<div
class="${this.includeScreenshot_ ? 'hidden' : 'screenshot-placeholder'}"
id="screenshot-placeholder">
<cr-icon icon="report_unsafe_site:visibility-off"></cr-icon>
</div>How about using conditional rendering instead? It seems much simpler thatn adding/removing a CSS class that shows/hides the element.
```suggestion
${!this.includeScreenshot_ ? html`
<div id="screenshot-placeholder">
<cr-icon icon="report_unsafe_site:visibility-off"></cr-icon>
</div>
` : ''}
```
Done
Incorrect indentation.
```suggestion
@change="${this.onIncludeScreenshotChange_}">
```
Done
Leverage this in tests to refer to the element, instead of using querySelector. Otherwise what is the point of adding this?
Done
await whenCheck(checkbox, () => checkbox.checked);Demetrios PapadopoulosWhy is this waiting necessary? Can you simply use `await microtasksFinished()` here? What triggers the checkbox to be checked?
FWIW, if this is triggered from the backend response callback, it is usually better to explicitly wait for the backend call to happen with something like
```
await this.handler.whenCalled(...);
await microtasksFinished();
```
than magically waiting for the state to change.
- Demetrios Papadopoulos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |