Refactor sidebar to use mwc list by donkawechico · Pull Request #7573 · home-assistant/frontend

donkawechico · 2020-11-01T17:56:50Z

Proposed change

Refactor sidebar to use mwc-list instead of paper-listbox.

Previous attempt: #7326
Related PR: #7453

Known Regressions

Issue	Screenshot	Status
Notification bell alignment regressed during the PR		Active
The closing "X" wiggles along with the list item text in edit mode:		Active
Clicking empty area in sidebar (e.g. space between normal panels and utility panels) moves focus to main content page rather than resetting focus on sidebar		Active, but you can just hit "Tab" to get back into list.

Fixed regressions

Issue	Screenshot	Status
~~In edit mode, you can only drag items by clicking in blank areas. Clicking on text or icon directly won't allow dragging.~~		Fixed
~~Text isn't bolded (is that a problem?)~~		Fixed
~~RTL Issues~~		Mostly fixed but had to use `!important` to do it. Remainder may not be blocking?
~~Slight border width inconsistency near utility panels:~~		Fixed
~~Arrow key navigation doesn't cycle to top from bottom or vice versa~~		Fixed
~~Hidden panel list isn't separated from rest of list~~	old screenshot	Fixed
~~List item highlight bar now spans full width (is that a problem?)~~	Not a problem. We should stick to mwc-list defaults	NAB
~~User Avatar circle too narrow~~	old screenshot	Fixed
~~Notification count badge explodes in collapsed view (and avatar circle is wrong):~~	old screenshot	Fixed
~~Icons are slightly too far to the left in collapsed view~~	old screenshot	Fixed

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New feature (thank you!)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example configuration

Additional information

This PR fixes or closes issue: fixes After navigating to another dashboard or switching browser tabs Quick Bar shortcut doesn't work until page clicked #7442
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
There is no commented out code in this PR.
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

src/components/ha-clickable-ripple.ts

donkawechico · 2020-11-02T02:56:15Z

src/components/ha-clickable-list-item.ts

+import { html } from "lit-html";
+
+@customElement("ha-clickable-list-item")
+export class HaClickableListItem extends ListItem {


mwc-list-item doesn't like being wrapped inside anchor tags:
material-components/material-web#1095 (comment)

We have to extend ListItem to retain keyboard navigation behavior

donkawechico · 2020-11-02T20:26:46Z

@zsarnett @bramkragten I think I'm ready for a first-pass review, and some code help if possible.

Ideally, if you could focus first on the known regressions list I laid out in the PR description, that would be great as those are the areas giving me the most problems.

I'm in the process of creating a 2nd cloud account for my local instance so you can eventually poke around on a live environment rather than pulling local. But it's taking forever for cloud to initiate so for now you should probably pull to poke around.

Thank you!

PS: I tried to remove as much unused CSS (and ts code that was only there for polymer) as I could but I'm sure there's more optimizations and simplifications that can be done. Feel free to point any out or ask if they're necessary.

src/resources/ha-sortable-style-ha-clickable.ts

src/components/ha-clickable-list-item.ts

bramkragten · 2020-11-29T15:52:25Z

src/components/ha-sidebar.ts

+        ${afterSpacer.map((panel) =>
+          this._renderPanel(
+            panel.url_path,
+            panel.url_path === this.hass.defaultPanel


We should check for component_name === lovelace here, and not for defaultPanel

Current master compares to defaultPanel. Is it wrong in master too then?

bramkragten · 2020-11-29T15:53:03Z

src/components/ha-sidebar.ts

+              ? panel.title || this.hass.localize("panel.states")
+              : this.hass.localize(`panel.${panel.title}`) || panel.title,
+            panel.icon,
+            panel.url_path === this.hass.defaultPanel && !panel.icon


Same here, panel.component_name === "lovelace"

It looks like this.hass.defaultPanel is set to the constant "lovelace" already here: https://github.com/home-assistant/frontend/blob/dev/src/fake_data/provide_hass.ts#L200

Should I still use an explicit string here or should I do panel.component_name === this.hass.defaultPanel

src/components/ha-sidebar.ts

bramkragten · 2020-11-29T16:23:11Z

src/components/ha-sidebar.ts

+    const elements = listbox.items;
+    const path = evt.composedPath();
+
+    for (const pathItem of path as Node[]) {
+      let index = -1;
+      if (isNodeElement(pathItem) && isListItem(pathItem)) {
+        index = elements.indexOf(pathItem);
+      }
+
+      if (index !== -1) {
+        return index;
+      }
+    }
+
+    return -1;


Why not use?

Suggested change

const elements = listbox.items;

const path = evt.composedPath();

for (const pathItem of path as Node[]) {

let index = -1;

if (isNodeElement(pathItem) && isListItem(pathItem)) {

index = elements.indexOf(pathItem);

}

if (index !== -1) {

return index;

}

}

return -1;

return listbox.getIndexOfTarget(evt);

Because we use a extended mwc-list-item with a different name? Let's make an extended mwc-list that handles this. It doesn't belong in ha-sidebar.

Yeah. That's probably for the best for a number of reasons, but going to be a non-trivial refactor. I'll get to that after Christmas then.

donkawechico · 2020-12-31T01:35:53Z

This got into a bad git state apparently. I've recreated it here: #8052

donkawechico added the Do Not Review PR is not ready for any kind of review label Nov 1, 2020

homeassistant added the cla-signed label Nov 1, 2020

donkawechico changed the title ~~Sidebar mwc list~~ Refactor sidebar to use mwc list Nov 1, 2020

donkawechico mentioned this pull request Nov 1, 2020

[Draft][Do not merge]Convert sidebar from paper-listbox to mwc-list #7326

Closed

9 tasks

bramkragten reviewed Nov 1, 2020

View reviewed changes

src/components/ha-clickable-ripple.ts Outdated Show resolved Hide resolved

donkawechico force-pushed the sidebar-mwc-list branch 4 times, most recently from 37df705 to 5f010c7 Compare November 2, 2020 02:52

donkawechico commented Nov 2, 2020

View reviewed changes

donkawechico force-pushed the sidebar-mwc-list branch 3 times, most recently from a92a697 to 6bdd397 Compare November 2, 2020 03:11

donkawechico removed the Do Not Review PR is not ready for any kind of review label Nov 2, 2020

donkawechico requested review from bramkragten and zsarnett November 2, 2020 20:26

donkawechico commented Nov 2, 2020

View reviewed changes

src/resources/ha-sortable-style-ha-clickable.ts Outdated Show resolved Hide resolved

donkawechico force-pushed the sidebar-mwc-list branch 3 times, most recently from 70c1751 to c5edb48 Compare November 16, 2020 21:22

donkawechico commented Nov 17, 2020

View reviewed changes

src/components/ha-clickable-list-item.ts Outdated Show resolved Hide resolved

donkawechico marked this pull request as ready for review November 20, 2020 18:29

bramkragten self-assigned this Nov 27, 2020

bramkragten reviewed Nov 29, 2020

View reviewed changes

src/components/ha-clickable-list-item.ts Outdated Show resolved Hide resolved

bramkragten reviewed Nov 29, 2020

View reviewed changes

src/components/ha-sidebar.ts Outdated Show resolved Hide resolved

bramkragten reviewed Nov 29, 2020

View reviewed changes

src/components/ha-sidebar.ts Outdated Show resolved Hide resolved

bramkragten reviewed Nov 29, 2020

View reviewed changes

donkawechico force-pushed the sidebar-mwc-list branch 2 times, most recently from 219f57f to 2048d9e Compare December 24, 2020 20:45

donkawechico closed this Dec 30, 2020

donkawechico force-pushed the sidebar-mwc-list branch from 14b89cf to 5409752 Compare December 30, 2020 22:26

donkawechico mentioned this pull request Dec 31, 2020

Sidebar mwc sidebar 2 #8052

Closed

9 tasks

github-actions bot locked and limited conversation to collaborators Jan 1, 2021

Conversation

donkawechico commented Nov 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Known Regressions

Fixed regressions

Type of change

Example configuration

Additional information

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donkawechico commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donkawechico commented Dec 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

donkawechico commented Nov 1, 2020 •

edited

Loading

donkawechico commented Nov 2, 2020 •

edited

Loading