Migrate DOMBreakpointsSidebarPane according to ui eng vision [devtools/devtools-frontend : main]
Kim-Anh Tran (Gerrit)
Kim-Anh Tran added 1 comment![]( "Open in Gerrit")
Hi Simon, ui eng migration, ptal! The web test needs to be updated, I'll disable it as soon as this CL is approved.
Related details
- Simon Zünd
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd voted and added 9 comments![]( "Open in Gerrit")
Votes added by Simon Zünd
| Code-Review | +1 |
9 comments
lgtm overall but with some suggestions
describeWithEnvironment('DOMBreakpointsSidebarPane', () => {nit: Use `describe` and call `setupLocaleHooks` instead.
describeWithMockConnection('DOMBreakpointsSidebarPane', () => {nit: Use `describe` and call `setupRuntimeHooks()` and `setupSettingsHooks()` and `setupLocaleHooks()` instead.
breakpoints: SDK.DOMDebuggerModel.DOMBreakpoint[];Alternative: Instead of passing the raw SDK `DOMBreakpoint` here we could have a small wrapper:
```
export interface Breakpoint {
breakpoint: SDK.DOMDebuggerModel.DOMBreakpoint;
label: string;
isHighlighted: boolean;
isFocused: boolean;
}
```
No strong opinion though if you prefer to handle this logic in the view function.
<div class=${hasBreakpoints ? 'hidden' : 'placeholder'}>Why use `hidden` at all? Do tests assume the old DOM structure? Why not
```
${hasBreakpoints
? html`<div class="placeholder">...</div>`
: html`<div class="brekapoint-list">...</div>`
}
```
const breakpointTypeLabel = BreakpointTypeLabels.get(breakpoint.type);nit: You could shortcut this: `BreakpointTypeLabels.get(breakpoint.type)?.() ?? ''`
private readonly breakpoints: SDK.DOMDebuggerModel.DOMBreakpoint[] = [];
private highlightedBreakpoint: SDK.DOMDebuggerModel.DOMBreakpoint|null = null;
private focusedBreakpoint: SDK.DOMDebuggerModel.DOMBreakpoint|null = null;
private readonly view: View;
nit: Lets keep private fields here and not move to the `private` keyword.
if (this.focusedBreakpoint && !this.breakpoints.includes(this.focusedBreakpoint)) {
this.focusedBreakpoint = null;
}
if (!this.focusedBreakpoint && this.breakpoints.length > 0) {
this.focusedBreakpoint = this.breakpoints[0];
}Having this logic in `performUpdate` is odd. Any chance we can move this into the the `set` accessor `focusedBreakpoint` or handle these when we modify `this.breakpoints`?
update(): void {This function is brittle because every exit path needs to remember to call `this.requestUpdate`. I think a better approach would be to have a set accessor for `highlightedBreakpoint` and `focusedBreakpoint` that calls `requestUpdate` when set automatically.
Related details
- Kim-Anh Tran
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kim-Anh Tran (Gerrit)
Kim-Anh Tran added 9 comments![]( "Open in Gerrit")
Hi Simon, I've updated the code, ptal 😊!
describeWithEnvironment('DOMBreakpointsSidebarPane', () => {nit: Use `describe` and call `setupLocaleHooks` instead.
Done
describeWithMockConnection('DOMBreakpointsSidebarPane', () => {nit: Use `describe` and call `setupRuntimeHooks()` and `setupSettingsHooks()` and `setupLocaleHooks()` instead.
Ah, noted, thanks!
Alternative: Instead of passing the raw SDK `DOMBreakpoint` here we could have a small wrapper:
```
export interface Breakpoint {
breakpoint: SDK.DOMDebuggerModel.DOMBreakpoint;
label: string;
isHighlighted: boolean;
isFocused: boolean;
}
```No strong opinion though if you prefer to handle this logic in the view function.
Done
Why use `hidden` at all? Do tests assume the old DOM structure? Why not
```
${hasBreakpoints
? html`<div class="placeholder">...</div>`
: html`<div class="brekapoint-list">...</div>`
}
```
TBH, it is a bit long ago that I worked on the first version, and it indeed was connected to tests, but I don't remember the details anymore. In any case, we can remove that now! Thanks!
const breakpointTypeLabel = BreakpointTypeLabels.get(breakpoint.type);nit: You could shortcut this: `BreakpointTypeLabels.get(breakpoint.type)?.() ?? ''`
Done
private readonly breakpoints: SDK.DOMDebuggerModel.DOMBreakpoint[] = [];
private highlightedBreakpoint: SDK.DOMDebuggerModel.DOMBreakpoint|null = null;
private focusedBreakpoint: SDK.DOMDebuggerModel.DOMBreakpoint|null = null;
private readonly view: View;
nit: Lets keep private fields here and not move to the `private` keyword.
Ah sorry, missed this, thanks for pointing out!
if (this.focusedBreakpoint && !this.breakpoints.includes(this.focusedBreakpoint)) {
this.focusedBreakpoint = null;
}
if (!this.focusedBreakpoint && this.breakpoints.length > 0) {
this.focusedBreakpoint = this.breakpoints[0];
}Having this logic in `performUpdate` is odd. Any chance we can move this into the the `set` accessor `focusedBreakpoint` or handle these when we modify `this.breakpoints`?
Hmm you're right, I've added a new function now that synchronizes the focused breakpoint, ptal!
This function is brittle because every exit path needs to remember to call `this.requestUpdate`. I think a better approach would be to have a set accessor for `highlightedBreakpoint` and `focusedBreakpoint` that calls `requestUpdate` when set automatically.
Ahh good idea. I've added setters, but I'm keeping the access through #focusedBreakpoint. Let me know if you want me to add getters too.
Related details
- Simon Zünd
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |