Skip to content
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 85 additions & 48 deletions src/components/ha-sidebar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
mdiMenuOpen,
} from "@mdi/js";
import type { UnsubscribeFunc } from "home-assistant-js-websocket";
import type { CSSResultGroup, PropertyValues } from "lit";
import type { PropertyValues } from "lit";
import { css, html, LitElement, nothing } from "lit";
import {
customElement,
Expand Down Expand Up @@ -39,6 +39,7 @@ import type { UpdateEntity } from "../data/update";
import { updateCanInstall } from "../data/update";
import { showEditSidebarDialog } from "../dialogs/sidebar/show-dialog-edit-sidebar";
import { SubscribeMixin } from "../mixins/subscribe-mixin";
import { ScrollableFadeMixin } from "../mixins/scrollable-fade-mixin";
import { actionHandler } from "../panels/lovelace/common/directives/action-handler-directive";
import { haStyleScrollbar } from "../resources/styles";
import type { HomeAssistant, PanelInfo, Route } from "../types";
Expand All @@ -59,8 +60,6 @@ const SORT_VALUE_URL_PATHS = {
map: 2,
logbook: 3,
history: 4,
"developer-tools": 9,
config: 11,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the dev tools and setting at the bottom of the full list as before, this makes them front and center when you have a lot of items in the sidebar. Devtools especially should not be at the top.

The scrolling (+ fade) should start on the divider between notifs and the full list

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the dev tools and setting at the bottom of the full list as before, this makes them front and center when you have a lot of items in the sidebar. Devtools especially should not be at the top.

Sorry, unclear. The main idea is that users may have plenty of dashboards (not to mention Energy, Calendar, History etc), and Dev tools & Settings are main system pages to manage/test settings. As a user, I use dev tools very very often, so I do not want to scroll down a list to find a needed item.
I excluded these 2 lines from ("9" & "11") since these items (dev tools & settings) will not be a part of sortable pages, they will belong to a different list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just marked here for reference, makes sense that these are not sortable.

As a user, I use dev tools very very often, so I do not want to scroll down a list to find a needed item.

This will not be strictly true for everyone. For this PR it may be best to stick to the fade changes and we can revisit this later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this PR it may be best to stick to the fade changes and we can revisit this later

I see, but this will be just a tiny part of the changes.
There are more fixes, not only related to "dev tools" & "settings".

Also, perhaps we should consult with UX guys regarding "dev tools" & "settings".
Note that this proposal will not affect setups with a small number of entries on a sidebar, it will only be visible with a large number of entries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the other fixes are good too.

I'll add the label for design to add their thoughts

};

const panelSorter = (
Expand Down Expand Up @@ -175,7 +174,7 @@ export const computePanels = memoizeOne(
);

@customElement("ha-sidebar")
class HaSidebar extends SubscribeMixin(LitElement) {
class HaSidebar extends SubscribeMixin(ScrollableFadeMixin(LitElement)) {
@property({ attribute: false }) public hass!: HomeAssistant;

@property({ type: Boolean, reflect: true }) public narrow = false;
Expand Down Expand Up @@ -207,6 +206,12 @@ class HaSidebar extends SubscribeMixin(LitElement) {

@query(".tooltip") private _tooltip!: HTMLDivElement;

@query(".before-spacer") private _scrollableList?: HTMLDivElement;

protected get scrollableElement(): HTMLElement | null {
return this._scrollableList as HTMLElement | null;
}

public hassSubscribe() {
return [
subscribeFrontendUserData(
Expand Down Expand Up @@ -267,22 +272,21 @@ class HaSidebar extends SubscribeMixin(LitElement) {
${this._renderNotifications()}
${this._renderUserItem(selectedPanel)}
</ha-md-list>
<div disabled class="bottom-spacer"></div>
<div class="tooltip"></div>
`;
<div class="tooltip"></div>`;
}

protected shouldUpdate(changedProps: PropertyValues): boolean {
if (
changedProps.has("expanded") ||
changedProps.has("narrow") ||
changedProps.has("alwaysExpand") ||
changedProps.has("_externalConfig") ||
changedProps.has("_updatesCount") ||
changedProps.has("_issuesCount") ||
changedProps.has("_notifications") ||
changedProps.has("_hiddenPanels") ||
changedProps.has("_panelOrder")
changedProps.has("_panelOrder") ||
changedProps.has("_contentScrolled") ||
changedProps.has("_contentScrollable")
) {
return true;
}
Expand Down Expand Up @@ -415,23 +419,41 @@ class HaSidebar extends SubscribeMixin(LitElement) {
this.hass.locale
);

return html`
<ha-md-list
class="ha-scrollbar"
const commonListPart = (_content, _class, scrollable: boolean) =>
html`<ha-md-list
class=${classMap({
"ha-scrollbar": scrollable,
[_class]: true,
})}
@focusin=${this._listboxFocusIn}
@focusout=${this._listboxFocusOut}
@touchend=${this._listboxTouchend}
@scroll=${this._listboxScroll}
@keydown=${this._listboxKeydown}
>
${this._renderPanels(beforeSpacer, selectedPanel)}
${this._renderSpacer()}
${this._renderPanels(afterSpacer, selectedPanel)}
${this.hass.user?.is_admin
? this._renderConfiguration(selectedPanel)
: this._renderExternalConfiguration()}
</ha-md-list>
`;
>${_content}</ha-md-list
>`;

return html`<div class="panels-list">
<div class="wrapper">
${commonListPart(
this._renderPanels(beforeSpacer, selectedPanel),
"before-spacer",
true
)}
${this.renderScrollableFades()}
</div>
${this._renderSpacer()}
${commonListPart(
html`
${this._renderPanels(afterSpacer, selectedPanel)}
${this.hass.user?.is_admin
? this._renderConfiguration(selectedPanel)
: this._renderExternalConfiguration()}
`,
"after-spacer",
false
)}
</div>`;
}

private _renderPanels(panels: PanelInfo[], selectedPanel: string) {
Expand Down Expand Up @@ -487,21 +509,17 @@ class HaSidebar extends SubscribeMixin(LitElement) {
<ha-svg-icon slot="start" .path=${mdiCog}></ha-svg-icon>
${!this.alwaysExpand &&
(this._updatesCount > 0 || this._issuesCount > 0)
? html`
<span class="badge" slot="start">
${this._updatesCount + this._issuesCount}
</span>
`
? html`<span class="badge" slot="start"
>${this._updatesCount + this._issuesCount}</span
>`
: nothing}
<span class="item-text" slot="headline"
>${this.hass.localize("panel.config")}</span
>
${this.alwaysExpand && (this._updatesCount > 0 || this._issuesCount > 0)
? html`
<span class="badge" slot="end"
>${this._updatesCount + this._issuesCount}</span
>
`
? html`<span class="badge" slot="end"
>${this._updatesCount + this._issuesCount}</span
>`
: nothing}
</ha-md-list-item>
`;
Expand All @@ -522,9 +540,7 @@ class HaSidebar extends SubscribeMixin(LitElement) {
>
<ha-svg-icon slot="start" .path=${mdiBell}></ha-svg-icon>
${!this.alwaysExpand && notificationCount > 0
? html`
<span class="badge" slot="start"> ${notificationCount} </span>
`
? html`<span class="badge" slot="start">${notificationCount}</span>`
: nothing}
<span class="item-text" slot="headline"
>${this.hass.localize("ui.notification_drawer.title")}</span
Expand Down Expand Up @@ -557,9 +573,9 @@ class HaSidebar extends SubscribeMixin(LitElement) {
.user=${this.hass.user}
.hass=${this.hass}
></ha-user-badge>
<span class="item-text" slot="headline">
${this.hass.user ? this.hass.user.name : ""}
</span>
<span class="item-text" slot="headline"
>${this.hass.user ? this.hass.user.name : ""}</span
>
</ha-md-list-item>
`;
}
Expand All @@ -576,9 +592,9 @@ class HaSidebar extends SubscribeMixin(LitElement) {
@mouseleave=${this._itemMouseLeave}
>
<ha-svg-icon slot="start" .path=${mdiCellphoneCog}></ha-svg-icon>
<span class="item-text" slot="headline">
${this.hass.localize("ui.sidebar.external_app_configuration")}
</span>
<span class="item-text" slot="headline"
>${this.hass.localize("ui.sidebar.external_app_configuration")}</span
>
</ha-md-list-item>
`;
}
Expand Down Expand Up @@ -705,8 +721,9 @@ class HaSidebar extends SubscribeMixin(LitElement) {
fireEvent(this, "hass-toggle-menu");
}

static get styles(): CSSResultGroup {
static get styles() {
return [
...super.styles,
haStyleScrollbar,
css`
:host {
Expand Down Expand Up @@ -778,11 +795,16 @@ class HaSidebar extends SubscribeMixin(LitElement) {
}

ha-fade-in,
ha-md-list {
.panels-list {
height: calc(
100% - var(--header-height) - var(--safe-area-inset-top, 0px) -
132px
);
100vh - var(--header-height) - var(--safe-area-inset-top, 0px) -
116px
); /* 116px = two list items (112px) + divider (4px) */
}

.panels-list {
display: flex;
flex-direction: column;
}

ha-fade-in {
Expand All @@ -799,6 +821,20 @@ class HaSidebar extends SubscribeMixin(LitElement) {
margin-left: var(--safe-area-inset-left, 0px);
}

.wrapper {
position: relative;
display: flex;
flex-direction: column;
min-height: 0;
}
ha-md-list.before-spacer {
padding-bottom: 0;
}
ha-md-list.after-spacer {
padding-top: 0;
min-height: fit-content;
}

ha-md-list-item {
flex-shrink: 0;
box-sizing: border-box;
Expand Down Expand Up @@ -862,15 +898,16 @@ class HaSidebar extends SubscribeMixin(LitElement) {
}

.divider {
bottom: 112px;
padding: 10px 0;
bottom: 112px; /* two list items (96px) + padding (16px) */
padding-bottom: 3px; /* makes a height = 4px */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this one a little better? What exactly is the bottom padding 3px for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A divider has a border = 1px, together with 3px it will be 4px (a spacing between 2 lists).

}
.divider::before {
content: " ";
display: block;
height: 1px;
background-color: var(--divider-color);
}

.badge {
display: flex;
justify-content: center;
Expand Down Expand Up @@ -909,7 +946,7 @@ class HaSidebar extends SubscribeMixin(LitElement) {
}

.spacer {
flex: 1;
margin-top: auto;
pointer-events: none;
}

Expand Down
Loading