Support Tool: Migrate url-generator to Lit [chromium/src : main]
Rebekah Potter (Gerrit)
Rebekah Potter voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rebekah Potter
| Commit-Queue | +1 |
1 comment
dpapad@ for Lit migration context, iremuguz@ as code OWNER.
Related details
- Demetrios Papadopoulos
- Irem Uguz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos voted and added 7 comments![]( "Open in Gerrit")
Votes added by Demetrios Papadopoulos
| Code-Review | +1 |
7 comments
LGTM with a couple nits and some minor questions.
private hasDataCollectorSelected(): boolean {
return this.dataCollectors_.some(collector => collector.isIncluded);
}Nit: Can probably inline this at line 85 directly now that is just a single line anyway?
protected onCopyUrlClick_() {
this.browserProxy_.generateCustomizedUrl(this.caseId_, this.dataCollectors_)
.then(this.onUrlGenerationResult_.bind(this));
}
protected onCopyTokenClick_() {
this.browserProxy_.generateSupportToken(this.dataCollectors_)
.then(this.onTokenGenerationResult_.bind(this));
}Optional: Use async/await?
this.dataCollectors_ = this.dataCollectors_.map(
item => ({...item, isIncluded: this.selectAll_}));Optional: Not sure if we have a preferred approach between the "immutable data pattern" or "mutate in-place and call `requestUpdate()`" approaches. Alternatively this could be as follows
```suggestion
for (const collector of this.dataCollectors_) {
collector.isIncluded = this.selectAll_;
}
this.requestUpdate();
```
this.onDataCollectorItemChange_();Having to call this manually is a bit against Lit reactive properties main philosophy of "reacting" to other state changes? Does this better belong as an "observer" in `willUpdate()` (possibly **not** guarded by any `changedProperties.has(...)` check to allow updating even when in-place updates happen) ?
this.onDataCollectorItemChange_();Same question here about whether calling this manually is aligned with Lit's philosophy.
const copyLinkButton = urlGenerator.shadowRoot.getElementById(
'copyURLButton')! as CrButtonElement;Let's change to `querySelector<TypeGoesHere>(...)` instead (here and elsewhere)?
Related details
- Irem Uguz
- Rebekah Potter
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter voted and added 6 comments![]( "Open in Gerrit")
Votes added by Rebekah Potter
| Commit-Queue | +1 |
6 comments
private hasDataCollectorSelected(): boolean {
return this.dataCollectors_.some(collector => collector.isIncluded);
}Nit: Can probably inline this at line 85 directly now that is just a single line anyway?
Done
protected onCopyUrlClick_() {
this.browserProxy_.generateCustomizedUrl(this.caseId_, this.dataCollectors_)
.then(this.onUrlGenerationResult_.bind(this));
}
protected onCopyTokenClick_() {
this.browserProxy_.generateSupportToken(this.dataCollectors_)
.then(this.onTokenGenerationResult_.bind(this));
}Rebekah PotterOptional: Use async/await?
Done
this.dataCollectors_ = this.dataCollectors_.map(
item => ({...item, isIncluded: this.selectAll_}));Optional: Not sure if we have a preferred approach between the "immutable data pattern" or "mutate in-place and call `requestUpdate()`" approaches. Alternatively this could be as follows
```suggestion
for (const collector of this.dataCollectors_) {
collector.isIncluded = this.selectAll_;
}
this.requestUpdate();
```
Done. I think it should be consistent and we do modify in place elsewhere in this file.
I've removed the requestUpdate() call since this is only called from within willUpdate().
Having to call this manually is a bit against Lit reactive properties main philosophy of "reacting" to other state changes? Does this better belong as an "observer" in `willUpdate()` (possibly **not** guarded by any `changedProperties.has(...)` check to allow updating even when in-place updates happen) ?
Done
Same question here about whether calling this manually is aligned with Lit's philosophy.
Done
const copyLinkButton = urlGenerator.shadowRoot.getElementById(
'copyURLButton')! as CrButtonElement;Let's change to `querySelector<TypeGoesHere>(...)` instead (here and elsewhere)?
it's not conditionally rendered, so I added it to the interface instead. Same for caseIdInput below.
Related details
- Irem Uguz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos voted and added 1 comment![]( "Open in Gerrit")
Votes added by Demetrios Papadopoulos
| Code-Review | +1 |
1 comment
}`this.requestUpdate()` call missing?
Related details
- Irem Uguz
- Rebekah Potter
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter added 1 comment![]( "Open in Gerrit")
`this.requestUpdate()` call missing?
This is only called from within willUpdate(), so I think this should be unnecessary (the element is already about to render); WDYT?
Related details
- Demetrios Papadopoulos
- Irem Uguz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 1 comment![]( "Open in Gerrit")
Rebekah Potter`this.requestUpdate()` call missing?
This is only called from within willUpdate(), so I think this should be unnecessary (the element is already about to render); WDYT?
Ah I see, didn't realize that, and since it was named 'on...' I assumed was an event listener.
Optional: Maybe inline this in `willUpdate()` since it is not called anywhere else? This would reduce some of the indirection that led to my confusion.
Related details
- Irem Uguz
- Rebekah Potter
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |