Migrate object_ui popovers towards view-based rendering [devtools/devtools-frontend : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
reusing issue, hope its correct, its non public
const DEFAULT_CUSTOM_PREVIEW_COMPONENT_VIEW: CustomPreviewComponentView = (input, _output, target) => {Helmut JanuschkaWe want the view to be injected into the constructor, so that the presenter logic could be tested in isolation.
Done.
OBJECT_POPOVER_CONTENT_VIEW(Helmut JanuschkaIf this is an intermediate step, this might be fine. But eventually we want the view to be injectable into a presenter.
Done
Related details
- Danil Somsikov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
Checkout https://chromium.googlesource.com/devtools/devtools-frontend/+/HEAD/docs/ui_engineering.md#examples for some inspiration. Specifically, the pattern should:
- Call the view function in it's performUpdate() function.
- Pass state as explicit setters, not the constructor.
- The constructor's signature should be constructor(target?: HTMLElement, view = DEFAULT_VIEW).
Related details
- Danil Somsikov
- Helmut Januschka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
reusing issue, hope its correct, its non public
407752215 is the correct one.
Related details
- Danil Somsikov
- Helmut Januschka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Checkout https://chromium.googlesource.com/devtools/devtools-frontend/+/HEAD/docs/ui_engineering.md#examples for some inspiration. Specifically, the pattern should:
- Call the view function in it's performUpdate() function.
- Pass state as explicit setters, not the constructor.
- The constructor's signature should be constructor(target?: HTMLElement, view = DEFAULT_VIEW).
thanks, guess this CL again needs a 3 way dance :/
are there any plans on improving this xp, pretty painfull in the end?
Related details
- Danil Somsikov
- Philip Pfaffe
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 3 comments![]( "Open in Gerrit")
I'm a little bit surprised the linter accepts this, the content is still assmebled entireley imperatively. @d...@chromium.org do we want to fix that?
#render(): void {Check out the examples here: https://chromium.googlesource.com/devtools/devtools-frontend/+/HEAD/docs/ui_engineering.md#examples
The view function should be called in performUpdate() and (mostly) only in performUpdate(). The right way to trigger an update (for CustomPreviewSection), is via this.requestUpdate(). This makes sure the lifecycle is as expected.
export const DEFAULT_OBJECT_POPOVER_PRESENTER_VIEWS: ObjectPopoverPresenterViews = {It's probably simpler to just have one Widget per type, WDYT?
Related details
- Danil Somsikov
- Helmut Januschka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov added 1 comment![]( "Open in Gerrit")
I'm a little bit surprised the linter accepts this, the content is still assmebled entireley imperatively. @d...@chromium.org do we want to fix that?
- Helmut Januschka
- Philip Pfaffe
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 2 comments![]( "Open in Gerrit")
Check out the examples here: https://chromium.googlesource.com/devtools/devtools-frontend/+/HEAD/docs/ui_engineering.md#examples
The view function should be called in performUpdate() and (mostly) only in performUpdate(). The right way to trigger an update (for CustomPreviewSection), is via this.requestUpdate(). This makes sure the lifecycle is as expected.
done, thanks for the details!
export const DEFAULT_OBJECT_POPOVER_PRESENTER_VIEWS: ObjectPopoverPresenterViews = {It's probably simpler to just have one Widget per type, WDYT?
Done refactored to:
- `ObjectPopoverContentWidget`
- `PrimitivePopoverWidget`
- `DescriptionPopoverWidget`
Related details
- Philip Pfaffe
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
content(): Node[] {This is still using imperative DOM all the way dow I'm afraid. So more things for the linter to catch!
Related details
- Helmut Januschka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov added 1 comment![]( "Open in Gerrit")
content(): Node[] {This is still using imperative DOM all the way dow I'm afraid. So more things for the linter to catch!
Of course, this patch is cheating by replacing `document.createTextNode` with `new Text` and `document.createElement` with `document.createElementNS`.
I will update the linter, but this is not what we normally have in the codebase.
Related details
- Helmut Januschka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Danil SomsikovThis is still using imperative DOM all the way dow I'm afraid. So more things for the linter to catch!
Of course, this patch is cheating by replacing `document.createTextNode` with `new Text` and `document.createElement` with `document.createElementNS`.
I will update the linter, but this is not what we normally have in the codebase.
ok thanks, should be ok now, @d...@chromium.org is the linter also somewhere opensource?
Related details
- Danil Somsikov
- Philip Pfaffe
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov added 1 comment![]( "Open in Gerrit")
content(): Node[] {Danil SomsikovThis is still using imperative DOM all the way dow I'm afraid. So more things for the linter to catch!
Helmut JanuschkaOf course, this patch is cheating by replacing `document.createTextNode` with `new Text` and `document.createElement` with `document.createElementNS`.
I will update the linter, but this is not what we normally have in the codebase.
ok thanks, should be ok now, @d...@chromium.org is the linter also somewhere opensource?
Related details
- Danil Somsikov
- Philip Pfaffe
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 13 comments![]( "Open in Gerrit")
className?: string;Class names are a view responsibility.
function joinClassNames(...classNames: Array<string|undefined>): string|undefined {Use classMap instead
function renderCustomPreviewSection(Should we just use <details><summary> instead here?
fallbackContent: Element|null;fallbackContent should not be part of the input.
onSectionHeaderClick: (event: Event) => void;We shouldn't be passing dom events out to the presenter
export type View = (input: ViewInput, output: ViewOutput, target: ShadowRoot) => void;output: undefined or output: {} is fine
constructor(object: SDK.RemoteObject.RemoteObject, onContentChanged: () => void = () => {}) {Do we need the callback? If so, let's use an event instead.
private onClick(event: Event): void {We should not handle dom events in the presenter.
this.defaultBodyTreeOutline = new ObjectPropertiesSectionsTreeOutline({readOnly: true});We should be using <devtools-tree> for this. PropertiesWidget could be a good example.
this.bodyUsesDefaultTreeOutline = true;This is a view responsibility and shouldn't be decided in the presenter.
this.customPreviewSection = new CustomPreviewSection(object, () => this.performUpdate());Use requestUpdate. if the web tests can't deal with that they need to be updated.
this.#shadowRoot = UI.UIUtils.createShadowRootWithCoreStyles(this.element, {cssFile: customPreviewComponentStyles});Presenters should not call this, but set flag in super() and apply styles in the view.
Related details
- Danil Somsikov
- Helmut Januschka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 14 comments![]( "Open in Gerrit")
A first round of comments!
Done
Class names are a view responsibility.
Done. Removed `className` from the `RenderedJSONMLElement` data model.
function joinClassNames(...classNames: Array<string|undefined>): string|undefined {Helmut JanuschkaUse classMap instead
Done
Should we just use <details><summary> instead here?
Done. Switched the expandable section to a native `<details>`/`<summary>`.
fallbackContent should not be part of the input.
Done. Removed `fallbackContent`
We shouldn't be passing dom events out to the presenter
Done
export type View = (input: ViewInput, output: ViewOutput, target: ShadowRoot) => void;output: undefined or output: {} is fine
Done
constructor(object: SDK.RemoteObject.RemoteObject, onContentChanged: () => void = () => {}) {Do we need the callback? If so, let's use an event instead.
done, callback is needed.
We should not handle dom events in the presenter.
Done
this.defaultBodyTreeOutline = new ObjectPropertiesSectionsTreeOutline({readOnly: true});We should be using <devtools-tree> for this. PropertiesWidget could be a good example.
Done
This is a view responsibility and shouldn't be decided in the presenter.
Done
this.customPreviewSection = new CustomPreviewSection(object, () => this.performUpdate());Use requestUpdate. if the web tests can't deal with that they need to be updated.
Done
this.#shadowRoot = UI.UIUtils.createShadowRootWithCoreStyles(this.element, {cssFile: customPreviewComponentStyles});Presenters should not call this, but set flag in super() and apply styles in the view.
- Danil Somsikov
- Philip Pfaffe
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |