Extract the header from ConsoleInsights as a new Widget [devtools/devtools-frontend : main]
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
export class ConsoleInsightHeader extends UI.Widget.Widget {this 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?
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 1 comment![]( "Open in Gerrit")
export class ConsoleInsightHeader extends UI.Widget.Widget {this 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?
Only 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.
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 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?
Only 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.
This sounds like extra steps with limited benefits. Could we just turn the ConsoleInsight into a widget directly?
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 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.
This sounds like extra steps with limited benefits. Could we just turn the ConsoleInsight into a widget directly?
There 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.
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 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?
There 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.
I 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.
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. |
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.
I 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.
Let me know if it's ready for review, I can do a full review.
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 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.
Let me know if it's ready for review, I can do a full review.
I 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.
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. |