fix(shell): publish the active view, so both navs follow the back path
The nav components learned where the user was from the `navigate` CustomEvent, which only the outbound path dispatches: `popstate` calls `handleNavigate()` directly. So a back-navigation left both of them highlighting the view just left — desktop included, at any width, on any back across two primary views. Opening a detail view was the same cause wearing a different symptom: `app-sidebar` guarded on its own item list and kept its highlight, `bottom-nav` did not and lit nothing. It cannot be fixed by re-dispatching `navigate` — `index.ts` is that event's document listener, so that is an infinite loop, and "please go to X" is not the statement being made. `activeViewStore` is the shell saying "the active view is now X", once per navigation, `popstate` included; both navs read it through a controller and hold no `activeView` of their own. A store rather than an event because a component that mounts *after* a navigation still has to know: `bottom-nav`'s drawer builds its `app-sidebar` on open, and that copy had heard nothing at all, so the drawer opened on Home from any page in the app. Closes #72
This commit is contained in:
@@ -7,6 +7,7 @@ import { designTokens } from '../../styles/tokens.css';
|
||||
import '../sidebar/app-sidebar.js';
|
||||
import { nameDialog } from '@utils/name-dialog';
|
||||
import { ICON_PLAYLIST } from '@utils/icon-language';
|
||||
import { ActiveViewController } from '@store/controllers/active-view-controller';
|
||||
|
||||
type View = 'home' | 'albums' | 'tracks' | 'playlists';
|
||||
|
||||
@@ -114,8 +115,19 @@ export class BottomNav extends LitElement {
|
||||
}
|
||||
`];
|
||||
|
||||
@state()
|
||||
private activeView = 'home';
|
||||
/**
|
||||
* Which tab is lit, read from the shell rather than tracked here.
|
||||
*
|
||||
* This was a `@state()` field set from the `navigate` event, which
|
||||
* only the outbound path dispatches -- so backing out of a detail
|
||||
* view left the highlight wherever it had been (#72). It had no
|
||||
* equivalent of `app-sidebar`'s `navItems.some(...)` guard either,
|
||||
* so a detail view set it to a name matching no tab and *nothing*
|
||||
* was lit; that asymmetry is why one nav looked broken and the
|
||||
* other looked fine. The store answers both: a detail view leaves
|
||||
* the tab it was opened from lit, in both components.
|
||||
*/
|
||||
private activeCtrl = new ActiveViewController(this);
|
||||
|
||||
/**
|
||||
* Whether the drawer has been asked for.
|
||||
@@ -167,12 +179,9 @@ export class BottomNav extends LitElement {
|
||||
nameDialog(this.drawer);
|
||||
}
|
||||
|
||||
private onGlobalNavigate = (e: Event) => {
|
||||
const detail = (e as CustomEvent<{ view?: string }>).detail;
|
||||
|
||||
if (detail?.view) this.activeView = detail.view;
|
||||
|
||||
private onGlobalNavigate = () => {
|
||||
// A navigation from inside the drawer is the drawer's job done.
|
||||
// The highlight is not this listener's business any more.
|
||||
this.drawerOpen = false;
|
||||
};
|
||||
|
||||
@@ -206,9 +215,11 @@ export class BottomNav extends LitElement {
|
||||
<li>
|
||||
<button
|
||||
type="button"
|
||||
class=${this.activeView === tab.id ? 'active' : ''}
|
||||
class=${this.activeCtrl.isActive(tab.id)
|
||||
? 'active'
|
||||
: ''}
|
||||
data-testid="tab-${tab.id}"
|
||||
aria-current=${this.activeView === tab.id
|
||||
aria-current=${this.activeCtrl.isActive(tab.id)
|
||||
? 'page'
|
||||
: 'false'}
|
||||
@click=${() => this.navigate(tab.id)}
|
||||
|
||||
@@ -4,6 +4,7 @@ import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||
import { designTokens } from '../../styles/tokens.css';
|
||||
|
||||
import type { DragActiveDetail } from '@utils/drag-controller';
|
||||
import { ActiveViewController } from '@store/controllers/active-view-controller';
|
||||
import {
|
||||
ICON_PLAYLIST,
|
||||
ICON_AUTOTAG,
|
||||
@@ -159,11 +160,20 @@ export class AppSidebar extends LitElement {
|
||||
/** Delay in ms before a drag-hover triggers navigation. */
|
||||
private static readonly HOVER_NAV_DELAY = 600;
|
||||
|
||||
/** Home, because that is where `index.ts` now navigates on startup
|
||||
* (H-8). The sidebar does not hear a `navigate` it did not send,
|
||||
* so this default is what keeps `aria-current` honest on arrival. */
|
||||
@state()
|
||||
private activeView: View = 'home';
|
||||
/**
|
||||
* Which item is lit, read from the shell rather than tracked here.
|
||||
*
|
||||
* This used to be a `@state()` field defaulting to `home` -- the
|
||||
* landing view -- because "the sidebar does not hear a `navigate`
|
||||
* it did not send". That default was the only honest moment it
|
||||
* ever had: a back-navigation dispatches no `navigate`, so the
|
||||
* highlight stayed on the view the user had just left (#72), and
|
||||
* the copy of this component that `bottom-nav` mounts inside its
|
||||
* drawer opened on `home` from whatever page you were standing on.
|
||||
* The shell publishes the active view now, so there is nothing to
|
||||
* default and nothing to keep in step.
|
||||
*/
|
||||
private activeCtrl = new ActiveViewController(this);
|
||||
|
||||
@state()
|
||||
private isDragging = false;
|
||||
@@ -237,10 +247,6 @@ export class AppSidebar extends LitElement {
|
||||
'yj-drag-active',
|
||||
this.onDragActive as EventListener,
|
||||
);
|
||||
document.addEventListener(
|
||||
'navigate',
|
||||
this.onGlobalNavigate as EventListener,
|
||||
);
|
||||
}
|
||||
|
||||
override disconnectedCallback() {
|
||||
@@ -262,10 +268,6 @@ export class AppSidebar extends LitElement {
|
||||
'yj-drag-active',
|
||||
this.onDragActive as EventListener,
|
||||
);
|
||||
document.removeEventListener(
|
||||
'navigate',
|
||||
this.onGlobalNavigate as EventListener,
|
||||
);
|
||||
this.clearDragHoverTimer();
|
||||
}
|
||||
|
||||
@@ -282,8 +284,9 @@ export class AppSidebar extends LitElement {
|
||||
<nav aria-label="Main">
|
||||
<ul>
|
||||
${this.navItems.map((item) => {
|
||||
const active = this.activeCtrl.isActive(item.id);
|
||||
const classes = [
|
||||
this.activeView === item.id
|
||||
active
|
||||
? 'active'
|
||||
: '',
|
||||
this.dragHoverView === item.id
|
||||
@@ -299,7 +302,7 @@ export class AppSidebar extends LitElement {
|
||||
type="button"
|
||||
class=${classes}
|
||||
data-testid="nav-${item.id}"
|
||||
aria-current=${this.activeView === item.id
|
||||
aria-current=${active
|
||||
? 'page'
|
||||
: 'false'}
|
||||
@click=${() =>
|
||||
@@ -382,19 +385,6 @@ export class AppSidebar extends LitElement {
|
||||
private static readonly DROP_VIEWS: Set<View> =
|
||||
new Set(['playlists']);
|
||||
|
||||
/** Keeps the highlighted nav item in sync with navigation that
|
||||
* originates outside the sidebar itself (e.g. the launch-page
|
||||
* dispatch in index.ts). */
|
||||
private onGlobalNavigate = (
|
||||
e: CustomEvent<{ view?: string }>,
|
||||
) => {
|
||||
const view = e.detail.view;
|
||||
|
||||
if (view && this.navItems.some((item) => item.id === view)) {
|
||||
this.activeView = view as View;
|
||||
}
|
||||
};
|
||||
|
||||
private onDragActive = (
|
||||
e: CustomEvent<DragActiveDetail>,
|
||||
) => {
|
||||
@@ -460,7 +450,11 @@ export class AppSidebar extends LitElement {
|
||||
}
|
||||
|
||||
private navigate(view: View) {
|
||||
this.activeView = view;
|
||||
// No optimistic highlight: the shell answers, and it answers
|
||||
// synchronously in `handleNavigate` before it awaits anything.
|
||||
// Setting it here as well is the second opinion this fix
|
||||
// removes -- it is what let a click's highlight survive a
|
||||
// navigation the shell then handled differently.
|
||||
this.dispatchEvent(new CustomEvent('navigate', {
|
||||
detail: { view },
|
||||
bubbles: true,
|
||||
|
||||
@@ -0,0 +1,90 @@
|
||||
/**
|
||||
* Which primary view the app is showing.
|
||||
*
|
||||
* The shell has always known this -- `handleNavigate()` sets
|
||||
* `#main-content`'s `data-active-view` on every path, `_isBack`
|
||||
* included -- and never told anyone. The nav components learned it
|
||||
* from the `navigate` CustomEvent instead, which only the *outbound*
|
||||
* path dispatches: the `popstate` listener calls `handleNavigate()`
|
||||
* directly. So both navs kept highlighting the view you had just left
|
||||
* (#72).
|
||||
*
|
||||
* The fix cannot be a re-dispatch of `navigate`. `index.ts` is itself a
|
||||
* document listener for it, so emitting one from inside
|
||||
* `handleNavigate` is an infinite loop -- and the two statements are
|
||||
* different anyway: `navigate` means *please go to X*, and 28 call
|
||||
* sites across 18 files say it. This says *the active view is now X*,
|
||||
* which only the shell is in a position to say and only once per
|
||||
* navigation.
|
||||
*
|
||||
* Three things about it are load-bearing.
|
||||
*
|
||||
* **It is a store rather than an event**, because a component that
|
||||
* mounts *after* a navigation still has to know. `bottom-nav`'s "More"
|
||||
* drawer creates its `<app-sidebar>` on open, and that copy had heard
|
||||
* no `navigate` at all: standing on Albums, the drawer highlighted
|
||||
* Home -- its `activeView` default, which existed to match the landing
|
||||
* view and matched nothing else ever after. An event has no answer for
|
||||
* a listener that was not there; a value does.
|
||||
*
|
||||
* **A detail view is not a view here.** Opening one leaves the primary
|
||||
* view it was opened from lit, which is what #72 asks for and what
|
||||
* `app-sidebar` used to do by accident -- it guarded on
|
||||
* `navItems.some(...)`, so a name matching no item left its highlight
|
||||
* alone. `bottom-nav` had no such guard and so lit nothing on a detail
|
||||
* view. Neither was correct; the sidebar was stale-but-lucky, and
|
||||
* stating the rule once is what makes the two agree.
|
||||
*
|
||||
* **Whether a view is primary is the shell's fact, not this store's.**
|
||||
* `VIEW_TAGS` in `index.ts` is the list, and a copy of it here is a
|
||||
* second list to forget -- so the caller passes the answer it already
|
||||
* has rather than this file re-deriving it.
|
||||
*/
|
||||
|
||||
type Subscriber = () => void;
|
||||
|
||||
class ActiveViewStore {
|
||||
/** Empty until the shell's first navigation, which happens at
|
||||
* startup from `GetDefaultPage()`. Nothing is highlighted for that
|
||||
* moment, which is honest: the alternative is a written-down
|
||||
* default that is right only when the default page agrees with it. */
|
||||
private activeView = '';
|
||||
|
||||
private subscribers = new Set<Subscriber>();
|
||||
|
||||
/** The active primary view, e.g. `albums`. */
|
||||
get(): string {
|
||||
return this.activeView;
|
||||
}
|
||||
|
||||
isActive(view: string): boolean {
|
||||
return this.activeView !== '' && this.activeView === view;
|
||||
}
|
||||
|
||||
/**
|
||||
* Called by the shell on every navigation, `popstate` included.
|
||||
*
|
||||
* `isPrimary` is `view in VIEW_TAGS` at the call site: a detail
|
||||
* view reports itself and deliberately changes nothing, so the view
|
||||
* it was opened from stays lit until the user picks another one.
|
||||
*/
|
||||
setView(view: string, isPrimary: boolean): void {
|
||||
if (!isPrimary) return;
|
||||
if (view === this.activeView) return;
|
||||
|
||||
this.activeView = view;
|
||||
this.notify();
|
||||
}
|
||||
|
||||
subscribe(fn: Subscriber): () => void {
|
||||
this.subscribers.add(fn);
|
||||
|
||||
return () => this.subscribers.delete(fn);
|
||||
}
|
||||
|
||||
private notify(): void {
|
||||
this.subscribers.forEach((fn) => fn());
|
||||
}
|
||||
}
|
||||
|
||||
export const activeViewStore = new ActiveViewStore();
|
||||
@@ -0,0 +1,59 @@
|
||||
import type {
|
||||
ReactiveController,
|
||||
ReactiveControllerHost,
|
||||
} from 'lit';
|
||||
import { activeViewStore } from '../active-view-store';
|
||||
|
||||
/**
|
||||
* ActiveViewController connects a Lit component to the
|
||||
* ActiveViewStore.
|
||||
*
|
||||
* Usage in a component:
|
||||
*
|
||||
* private activeCtrl = new ActiveViewController(this);
|
||||
*
|
||||
* render() {
|
||||
* const lit = this.activeCtrl.isActive('albums');
|
||||
* }
|
||||
*
|
||||
* It reads through to the store rather than copying the value into a
|
||||
* `@state()` field, which is the point of #72: two components holding
|
||||
* their own idea of the active view is what let them disagree with the
|
||||
* shell and with each other.
|
||||
*/
|
||||
export class ActiveViewController implements ReactiveController {
|
||||
private host: ReactiveControllerHost;
|
||||
private unsubscribe?: () => void;
|
||||
|
||||
constructor(host: ReactiveControllerHost) {
|
||||
this.host = host;
|
||||
host.addController(this);
|
||||
}
|
||||
|
||||
// ===============================================================
|
||||
// LIFECYCLE HOOKS
|
||||
// ===============================================================
|
||||
|
||||
hostConnected(): void {
|
||||
this.unsubscribe = activeViewStore.subscribe(() => {
|
||||
this.host.requestUpdate();
|
||||
});
|
||||
}
|
||||
|
||||
hostDisconnected(): void {
|
||||
this.unsubscribe?.();
|
||||
}
|
||||
|
||||
// ===============================================================
|
||||
// DATA ACCESS
|
||||
// ===============================================================
|
||||
|
||||
/** The active primary view, e.g. `albums`. */
|
||||
get current(): string {
|
||||
return activeViewStore.get();
|
||||
}
|
||||
|
||||
isActive(view: string): boolean {
|
||||
return activeViewStore.isActive(view);
|
||||
}
|
||||
}
|
||||
@@ -9,6 +9,8 @@ export type { ThemeState, BackgroundShade } from './theme-store';
|
||||
export { ThemeController } from './controllers/theme-controller';
|
||||
export { searchStore } from './search-store';
|
||||
export { SearchController } from './controllers/search-controller';
|
||||
export { activeViewStore } from './active-view-store';
|
||||
export { ActiveViewController } from './controllers/active-view-controller';
|
||||
export { shortcutsStore } from './shortcuts-store';
|
||||
export type { ShortcutsState } from './shortcuts-store';
|
||||
export { ShortcutsController } from './controllers/shortcuts-controller';
|
||||
|
||||
Reference in New Issue
Block a user