profile-picker UI: Migrate to Lit, part 3. [chromium/src : main]
Teresa Mao (Gerrit)
Teresa Mao added 6 comments![]( "Open in Gerrit")
const routeObservers: Set<NavigationMixinInterface> = new Set();Teresa MaoThis needs to be shared across Lit and Polymer versions of the mixin.
This is what causes the add profile button issue. Thanks.
protected hideAskOnStartup_: boolean = true;Teresa MaoShould this be initialized to false?
It was initialized to true to match the default value in the Polymer version. See line 76 in the original file.
this.askOnStartup_ = loadTimeData.getBoolean('askOnStartup');Teresa MaoMerge with line 82 and remove the constructor?
Done
this.updateComplete.then(() => {Teresa MaoUsing the `updateComplete` promise within the `updated()` callback should not be necessary. Is thi sneeded?
Done
navigateTo(Routes.NEW_PROFILE);Demetrios Papadopoulos@dpa...@chromium.org I need help with debugging the "Add" profile button not being responsible when clicked.
Steps to reproduce:
- Go to chrome://profile-picker.
- Click the "Add" button next to the existing profile.
Expected: It should route to chrome://profile-picker/new-profile.
Actual: No visual changes.
What I know so far:
- `onAddProfileClick_()` is called which is triggered by button clicked.
- `navigateTo()` from navigation_mixin_lit.ts is reached.
- The call to notify routeObservers on line 161 of navigation_mixin_lit.ts is reached.
- `onRouteChange()` on line 161 of profile_picker_main_view.ts is reached, but no further action occurs.
Demetrios PapadopoulosI haven't had a chance to patch this CL locally yet. Will update once I have any findings.
Teresa MaoDoes the code at https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/resources/signin/profile_picker/profile_picker_app.ts;l=90;drc=3f4203f7dca2f7e804f30cfa783e24f90acd9059 execute? If not, that would be why there is nothing happening. See other related comment.
No, as it was pointed out in the other comment, profile-picker-app extends a different NavigationMixin class compared to profile-picker-main-view. This is why a `routeObservers` in one NavigationMixin class cannot notify the observers in the other class.
this.profilesList_ = [
...this.profilesList_.slice(0, index),
...this.profilesList_.slice(index + 1),
];Teresa MaoUnnecessary calculations? Does the following not work equivalently?
```suggestion
this.profilesList_.splice(i, 1);
this.requestUpdate();
```
If I do `this.profilesList_.splice(i, 1);`, Lit will not capture `profilesList_` in `changedProperties` in life cycle methods such as `willUpdate()`, and the computations depending on `profilesList_` will not be executed. To ensure Lit detects the change, I need to explicitly assign a new value to `profilesList_`.
To make your suggestion work, I will need to remove the if check around computations depending on `profilesList_`.
Related details
- Demetrios Papadopoulos
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 11 comments![]( "Open in Gerrit")
cc'ing a few more specific owners.
@temao: Were you able to locally verify the drag and drop behavior?
@owners: Is ProfilesReordering still relevant? The feature is DISABLED_BY_DEFAULT. Should this code be removed?
private titleList_: Object[];What's the actual type of this? Can we use a more accurate type here?
this.litElement_ = litElement;Let's rename to just `element`? I don't think adding 'polymer' or 'lit' in the name is very beneficial here.
this.tileListName_ = tileListName;Is this still used anywhere?
export const NavigationMixinLit = dedupingMixin(dedupingMixin shouldn't be used with the Lit version, no?
if (index !== -1) {Can this ever be -1? Should this be an assertion instead?
this.initializeDragDelegate_();Since we are already calling this on `updated()` we shouldn't have to call it here, no?
return !isAskOnStartupAllowed() || !this.profilesList_ ||Can this ever be true? If not let's remove.
this.profilesList_ = [
...this.profilesList_.slice(0, index),
...this.profilesList_.slice(index + 1),
];Teresa MaoUnnecessary calculations? Does the following not work equivalently?
```suggestion
this.profilesList_.splice(i, 1);
this.requestUpdate();
```
If I do `this.profilesList_.splice(i, 1);`, Lit will not capture `profilesList_` in `changedProperties` in life cycle methods such as `willUpdate()`, and the computations depending on `profilesList_` will not be executed. To ensure Lit detects the change, I need to explicitly assign a new value to `profilesList_`.
To make your suggestion work, I will need to remove the if check around computations depending on `profilesList_`.
I will need to remove the if check around computations depending on profilesList_.
Let's do this instead. For trivial (cheap) updates we don't necessarily have to use changeProperties to trigger them.
return this.shadowRoot!.querySelector<HTMLElement>('#profile-' + index)!;Since we now attach a data-index attribute to each element, can this be changed as follows?
profilesContainer
```suggestion
return this.shadowRoot!.querySelector<HTMLElement>('profile-card[data-index="${index}"]'!;
```
And remove the ID from the template?
Related details
- Teresa Mao
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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")
protected hideAskOnStartup_: boolean = true;Teresa MaoShould this be initialized to false?
It was initialized to true to match the default value in the Polymer version. See line 76 in the original file.
The initial value here is overwritten in the first willUpdate() call before the element has rendered, so I don't think the 'true' value here makes any difference. Even in the Polymer version, computed properties default value does not make a lot of sense
Monica Basta (Gerrit)
Monica Basta added 1 comment![]( "Open in Gerrit")
cc'ing a few more specific owners.
@temao: Were you able to locally verify the drag and drop behavior?
@owners: Is ProfilesReordering still relevant? The feature is DISABLED_BY_DEFAULT. Should this code be removed?
@rs...@google.com can you give more context about the launch state for `ProfilesReordering`? If I am not mistaken there are few corner cases that need to fixed before the launch but the team is focused on other priorities
Related details
- Demetrios Papadopoulos
- Ryan Sultanem
- Teresa Mao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Roger (Gerrit)
David Roger added 1 comment![]( "Open in Gerrit")
Monica Bastacc'ing a few more specific owners.
@temao: Were you able to locally verify the drag and drop behavior?
@owners: Is ProfilesReordering still relevant? The feature is DISABLED_BY_DEFAULT. Should this code be removed?
@rs...@google.com can you give more context about the launch state for `ProfilesReordering`? If I am not mistaken there are few corner cases that need to fixed before the launch but the team is focused on other priorities
We should keep this feature. We have active plans to launch it. I think the flag just expired, but we should re-enable it.
Related details
- Demetrios Papadopoulos
- Monica Basta
- Ryan Sultanem
- Teresa Mao
Ryan Sultanem (Gerrit)
Ryan Sultanem added 1 comment![]( "Open in Gerrit")
Monica Bastacc'ing a few more specific owners.
@temao: Were you able to locally verify the drag and drop behavior?
@owners: Is ProfilesReordering still relevant? The feature is DISABLED_BY_DEFAULT. Should this code be removed?
David Roger@rs...@google.com can you give more context about the launch state for `ProfilesReordering`? If I am not mistaken there are few corner cases that need to fixed before the launch but the team is focused on other priorities
We should keep this feature. We have active plans to launch it. I think the flag just expired, but we should re-enable it.
Indeed the feature is still relevant.
The feature launch was started, but then after finding some minor bugs (that needs fixing) and attempting to explore more ways to make the feature discoverable, we decided to leave it for later.
However the team got other priorities in the meantime which delayed the updates to the feature even more.
I have recently started looking into more and it should be completed and revived very soon.
Related details
- David Roger
- Demetrios Papadopoulos
- Monica Basta
- Teresa Mao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
- Demetrios Papadopoulos
- Monica Basta
- Teresa Mao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Teresa Mao (Gerrit)
Teresa Mao added 12 comments![]( "Open in Gerrit")
cc'ing a few more specific owners.
@temao: Were you able to locally verify the drag and drop behavior?
@owners: Is ProfilesReordering still relevant? The feature is DISABLED_BY_DEFAULT. Should this code be removed?
@temao: Were you able to locally verify the drag and drop behavior?
I tested drag and drop locally by removing if check in `initializeDragDelegate_()` which gets the value of feature flag `profilesReorderingEnabled` and is disabled by default. All the drag and drop browser tests (ProfilePickerTest.MainView) were run on both windows and lacros, and they all passed.
What's the actual type of this? Can we use a more accurate type here?
Done
Let's rename to just `element`? I don't think adding 'polymer' or 'lit' in the name is very beneficial here.
Done
Is this still used anywhere?
Removed.
export const NavigationMixinLit = dedupingMixin(dedupingMixin shouldn't be used with the Lit version, no?
Do we have a lit equivalent version of dedupingMixin?
protected hideAskOnStartup_: boolean = true;Teresa MaoShould this be initialized to false?
Demetrios PapadopoulosIt was initialized to true to match the default value in the Polymer version. See line 76 in the original file.
The initial value here is overwritten in the first willUpdate() call before the element has rendered, so I don't think the 'true' value here makes any difference. Even in the Polymer version, computed properties default value does not make a lot of sense
Done
Can this ever be -1? Should this be an assertion instead?
Done
this.initializeDragDelegate_();Since we are already calling this on `updated()` we shouldn't have to call it here, no?
If we remove it here, we will need to add a null check on line 105 of drag_drop_reorder_tile_list_delegate.ts. This is because relying on calling initializeDragDelegate_() in updated() will cause the DOM to be out of sync with the data passed in drag_drop_reorder_tile_list_delegate.ts, which will then lead to null exception in line 105 of drag_drop_reorder_tile_list_delegate.ts when it tries to access a tile which has already been removed.
return !isAskOnStartupAllowed() || !this.profilesList_ ||Can this ever be true? If not let's remove.
Done
this.profilesList_ = [
...this.profilesList_.slice(0, index),
...this.profilesList_.slice(index + 1),
];Teresa MaoUnnecessary calculations? Does the following not work equivalently?
```suggestion
this.profilesList_.splice(i, 1);
this.requestUpdate();
```
Demetrios PapadopoulosIf I do `this.profilesList_.splice(i, 1);`, Lit will not capture `profilesList_` in `changedProperties` in life cycle methods such as `willUpdate()`, and the computations depending on `profilesList_` will not be executed. To ensure Lit detects the change, I need to explicitly assign a new value to `profilesList_`.
To make your suggestion work, I will need to remove the if check around computations depending on `profilesList_`.
I will need to remove the if check around computations depending on profilesList_.Let's do this instead. For trivial (cheap) updates we don't necessarily have to use changeProperties to trigger them.
Done
return this.shadowRoot!.querySelector<HTMLElement>('#profile-' + index)!;Since we now attach a data-index attribute to each element, can this be changed as follows?
profilesContainer
```suggestion
return this.shadowRoot!.querySelector<HTMLElement>('profile-card[data-index="${index}"]'!;
```And remove the ID from the template?
Done
Related details
- Demetrios Papadopoulos
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |