healthd_internals: Add custom element for telemetry page [chromium/src : main]
Byron Lee (Gerrit)
Byron Lee has uploaded the change for review![]( "Open in Gerrit")
Commit message
healthd_internals: Add custom element for telemetry page
Also updated the CSS and fixed the path handling.
Change diff
Change information
- M chrome/browser/resources/chromeos/healthd_internals/BUILD.gn
- M chrome/browser/resources/chromeos/healthd_internals/app.html
- M chrome/browser/resources/chromeos/healthd_internals/app.ts
- M chrome/browser/resources/chromeos/healthd_internals/healthd_internals.html
- M chrome/browser/resources/chromeos/healthd_internals/healthd_internals_shared.css
- A chrome/browser/resources/chromeos/healthd_internals/pages/telemetry.html
- A chrome/browser/resources/chromeos/healthd_internals/pages/telemetry.ts
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Byron Lee (Gerrit)
Byron Lee voted Commit-Queue+1![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Cheng (Gerrit)
Ethan Cheng added 4 comments![]( "Open in Gerrit")
High level comment:
Mostly LGTM. If this work is non-urgent, can you help look into using decorators for writing polymer?
I'm not sure if this is supported in Ash, but using decorator should significantly simplify the codebase, and make the control flow much easier to view.
go/polymer-ts-style#declaring-properties
https://github.com/Polymer/polymer-decorators
export interface HealthdInternalsAppElement {
$: {
selector: CrMenuSelector,
};
}I believe this is used to query the selector element from its HTML template, and access it in TS.
However, I don't think it is used right now anymore, can you confirm whether we need this?
go/cgcm/styleguide/web/web.md#polymer
currentPath: {type: String},Just to confirm, does this now solve the problem of navigation from URL directly?
export interface HealthdInternalsTelemetryElement {
$: {};
}Ditto.
Can you confirm whether we need this?
We can always add it back in future CLs if you need to query internal elements.
go/cgcm/styleguide/web/web.md#polymer
Related details
- Byron Lee
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Cheng (Gerrit)
Ethan Cheng added 1 comment![]( "Open in Gerrit")
High level comment:
Mostly LGTM. If this work is non-urgent, can you help look into using decorators for writing polymer?
I'm not sure if this is supported in Ash, but using decorator should significantly simplify the codebase, and make the control flow much easier to view.
go/polymer-ts-style#declaring-properties
https://github.com/Polymer/polymer-decorators
NVM, a quick codesearch suggests that there's no decorator usage in place right now.
There were discussion a few years back and I don't think this is ported yet.
Related details
- Byron Lee
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Byron Lee (Gerrit)
Byron Lee voted and added 4 comments![]( "Open in Gerrit")
Votes added by Byron Lee
| Commit-Queue | +1 |
High level comment:
Mostly LGTM. If this work is non-urgent, can you help look into using decorators for writing polymer?
I'm not sure if this is supported in Ash, but using decorator should significantly simplify the codebase, and make the control flow much easier to view.
go/polymer-ts-style#declaring-properties
https://github.com/Polymer/polymer-decorators
That looks helpful!
However, I didn't find any [use cases](https://source.chromium.org/search?q=%22@customElement%22&sq=&ss=chromium%2Fchromium%2Fsrc:chrome%2F) in Chromium and any [decorator-related files](https://source.chromium.org/search?q=decorator&ss=chromium%2Fchromium%2Fsrc:third_party%2Fpolymer%2F&start=1) in polymer folders.
I guess it's not supported in Chromium.
export interface HealthdInternalsAppElement {
$: {
selector: CrMenuSelector,
};
}I believe this is used to query the selector element from its HTML template, and access it in TS.
However, I don't think it is used right now anymore, can you confirm whether we need this?
go/cgcm/styleguide/web/web.md#polymer
I think we don't need to keep it. Removed!
Just to confirm, does this now solve the problem of navigation from URL directly?
Yes, let me update the commit message.
export interface HealthdInternalsTelemetryElement {
$: {};
}Ditto.
Can you confirm whether we need this?
We can always add it back in future CLs if you need to query internal elements.
go/cgcm/styleguide/web/web.md#polymer
- Ethan Cheng
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Cheng (Gerrit)
Ethan Cheng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ethan Cheng
| Code-Review | +1 |
Byron LeeHigh level comment:
Mostly LGTM. If this work is non-urgent, can you help look into using decorators for writing polymer?
I'm not sure if this is supported in Ash, but using decorator should significantly simplify the codebase, and make the control flow much easier to view.
go/polymer-ts-style#declaring-properties
https://github.com/Polymer/polymer-decorators
That looks helpful!
However, I didn't find any [use cases](https://source.chromium.org/search?q=%22@customElement%22&sq=&ss=chromium%2Fchromium%2Fsrc:chrome%2F) in Chromium and any [decorator-related files](https://source.chromium.org/search?q=decorator&ss=chromium%2Fchromium%2Fsrc:third_party%2Fpolymer%2F&start=1) in polymer folders.
I guess it's not supported in Chromium.
Done
<iron-pages selected="[[selectedIndex]]">Not in this CL
Can you check the status of using iron-xxx elements?
I know that there was effort in chromium WebUI to deprecate all use of iron-xxx, replacing with either cr-element or something else.
Related details
- Byron Lee
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Byron Lee (Gerrit)
Byron Lee voted and added 1 comment![]( "Open in Gerrit")
Votes added by Byron Lee
| Commit-Queue | +2 |
1 comment
<iron-pages selected="[[selectedIndex]]">Not in this CL
Can you check the status of using iron-xxx elements?
I know that there was effort in chromium WebUI to deprecate all use of iron-xxx, replacing with either cr-element or something else.
Sure. I will check it later.
IIUC, Lit is not supported under `//chrome/browser/resources/chromeos/`, maybe the migration is not required for us.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
healthd_internals: Add custom element for telemetry page
Also updated the CSS and fixed the variable style and issue of
navigation from URL.
- M chrome/browser/resources/chromeos/healthd_internals/BUILD.gn
- M chrome/browser/resources/chromeos/healthd_internals/app.html
- M chrome/browser/resources/chromeos/healthd_internals/app.ts
- M chrome/browser/resources/chromeos/healthd_internals/healthd_internals.html
- M chrome/browser/resources/chromeos/healthd_internals/healthd_internals_shared.css
- A chrome/browser/resources/chromeos/healthd_internals/pages/telemetry.html
- A chrome/browser/resources/chromeos/healthd_internals/pages/telemetry.ts
Code-Review: +1 by Ethan Cheng
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |