Extract view from ConsoleInsights [devtools/devtools-frontend : main]
Piotr Paulski (Gerrit)
Piotr Paulski added 1 comment![]( "Open in Gerrit")
Related details
- Alex Rudenko
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 8 comments![]( "Open in Gerrit")
.data=${nit: .data is deprecated and I think we should use attributes
const directCitationUrls = state.directCitationUrls;this calculation block sounds like it might belong to the presentation logic rather than the view? let's accept relatedUrls on the view input?
const noLogging = Root.Runtime.hostConfig.aidaAvailability?.enterprisePolicyValue ===this logic should be in the present, let's add a noLogging view input param
Host.userMetrics.actionTaken(Host.UserMetrics.Action.InsightsOptInTeaserSettingsLinkClicked);let's move this to the presenter
const noLogging = Root.Runtime.hostConfig.aidaAvailability?.enterprisePolicyValue ===Also, here the noLogging calculation is duplicated
}}I think this should be in the presenter, not the view.
if (this.#referenceDetailsRef.value) {can we do it without a ref by passing a value to onToggleReferenceDetails?
Related details
- Piotr Paulski
- Wolfgang Beyer
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
export class ConsoleInsightHeader extends UI.Widget.Widget {Piotr Paulskithis presenter does not integrate with any models and has very little logic. I think this should be a view. Is there a reason to extract it?
Alex RudenkoOnly temporarily. When I rewrite all parts of ConsoleInsight to widgets, I will replace this and similar smaller components back to functions in the large view.
Piotr PaulskiThis sounds like extra steps with limited benefits. Could we just turn the ConsoleInsight into a widget directly?
Alex RudenkoThere are a lot of additional fixes needed to convert all the animations to not use imperative DOM manipulations and a few race conditions that show up after conversion to widget due to change from sync to async rendering. I felt I need to split it into several steps to make sure it is reviewable.
This first one actually solves couple of issues with component initialization order (by moving `#render` from the constructor) and unblocks this step-by-step rewrite. I updated CL description to point that out.
Alex RudenkoI am not sure it justifies the presenter. Why cannot it be a helper function producing a lit-html template? The presenter only adds unnecessary property passing and widget instantiations in my opinion.
Piotr PaulskiLet me know if it's ready for review, I can do a full review.
Alex RudenkoI came to the same conclusion overnight - I don't need the widget to convert part of the existing `#render` method to a function outside of the component. Just pass the inputs as parameter instead of relying on having access to `this.#something`. I'll let you know when I convert this CL.
Thanks! Mostly looks good to me, left a few suggestions.
Wolfgang Beyer (Gerrit)
Wolfgang Beyer added 1 comment![]( "Open in Gerrit")
I have nothing to add. Alex' suggestions of moving some logic to the presenter SGTM.
Related details
- Piotr Paulski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Piotr Paulski (Gerrit)
Piotr Paulski added 7 comments![]( "Open in Gerrit")
nit: .data is deprecated and I think we should use attributes
Done
this calculation block sounds like it might belong to the presentation logic rather than the view? let's accept relatedUrls on the view input?
Done
const noLogging = Root.Runtime.hostConfig.aidaAvailability?.enterprisePolicyValue ===this logic should be in the present, let's add a noLogging view input param
Done
Host.userMetrics.actionTaken(Host.UserMetrics.Action.InsightsOptInTeaserSettingsLinkClicked);let's move this to the presenter
Done
const noLogging = Root.Runtime.hostConfig.aidaAvailability?.enterprisePolicyValue ===Also, here the noLogging calculation is duplicated
Done
I think this should be in the presenter, not the view.
Done
export class ConsoleInsightHeader extends UI.Widget.Widget {Piotr Paulskithis presenter does not integrate with any models and has very little logic. I think this should be a view. Is there a reason to extract it?
Alex RudenkoOnly temporarily. When I rewrite all parts of ConsoleInsight to widgets, I will replace this and similar smaller components back to functions in the large view.
Piotr PaulskiThis sounds like extra steps with limited benefits. Could we just turn the ConsoleInsight into a widget directly?
Alex RudenkoThere are a lot of additional fixes needed to convert all the animations to not use imperative DOM manipulations and a few race conditions that show up after conversion to widget due to change from sync to async rendering. I felt I need to split it into several steps to make sure it is reviewable.
This first one actually solves couple of issues with component initialization order (by moving `#render` from the constructor) and unblocks this step-by-step rewrite. I updated CL description to point that out.
Alex RudenkoI am not sure it justifies the presenter. Why cannot it be a helper function producing a lit-html template? The presenter only adds unnecessary property passing and widget instantiations in my opinion.
Piotr PaulskiLet me know if it's ready for review, I can do a full review.
Alex RudenkoI came to the same conclusion overnight - I don't need the widget to convert part of the existing `#render` method to a function outside of the component. Just pass the inputs as parameter instead of relying on having access to `this.#something`. I'll let you know when I convert this CL.
Thanks! Mostly looks good to me, left a few suggestions.
- Alex Rudenko
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Piotr Paulski (Gerrit)
Piotr Paulski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Piotr Paulski
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
if (this.#referenceDetailsRef.value) {can we do it without a ref by passing a value to onToggleReferenceDetails?
Prefer to fix this in a separate CL: at render time the value is not known, details could be toggled multiple times between renders. I need to rewrite whole opening animation to get this straight.
Related details
- Alex Rudenko
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Code-Review | +1 |
1 comment
if (this.#referenceDetailsRef.value) {Piotr Paulskican we do it without a ref by passing a value to onToggleReferenceDetails?
Prefer to fix this in a separate CL: at render time the value is not known, details could be toggled multiple times between renders. I need to rewrite whole opening animation to get this straight.
- Piotr Paulski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wolfgang Beyer
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
Related details
- Piotr Paulski
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)
Devtools-frontend LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Extract view from ConsoleInsights
In preparation to applying UI Engineering vision to console insights,
replace all view-like methods with functions outside the component.
Next steps will be to deal with imperative DOM manipulations,
clean up state manipulation, then finally convert to Widget.
Bug: 407751694
Change-Id: I6299e0895089b64bfd3d90f485e2d7edff962bf3
BYPASS_LARGE_CHANGE_WARNING: Simple code move mostly.
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7106268
Auto-Submit: Piotr Paulski <piotrp...@chromium.org>
Reviewed-by: Wolfgang Beyer <wo...@chromium.org>
Reviewed-by: Alex Rudenko <alexr...@chromium.org>
Commit-Queue: Wolfgang Beyer <wo...@chromium.org>
- M front_end/panels/explain/components/ConsoleInsight.ts
Code-Review: +1 by Alex Rudenko, +1 by Wolfgang Beyer
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |