mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 01:54:10 +00:00
fix(sidebar): the open row no longer fades in from black on every load
Every sidebar row went out with no colour class, and ui--sidebar gave the open one bg-accent-soft and text-accent (and every other row text-ink-muted) on connect. By then the browser had already styled the page, and the rows carry transition-colors for their hover, so on every full load and every Turbo visit the open row faded in over 150ms from the inherited black and a transparent pill, and the rest faded from black to their muted ink. Measured in headless Chromium against the test stack: app.css loaded at ~155ms, first paint at ~200ms with the rows black, the controller's pass at ~250ms, transitionrun on every visible row in the next frame. The stylesheet order was never the cause, and neither was the per-frame sampler: plain transitionrun listeners see the same thing. The server now renders the state. SidebarRowStateExtension restates ui--sidebar#_matches against the main request, and every link row in the sidebar, the account list and an expanded account's folders takes its classes from sidebar_row_state(). The controller's pass is then a no-op on everything the server drew, the first style the browser resolves is the final one, and the hover transitions are untouched (still ~130ms in and out). The pass stays for frame navigations and for rows that arrive later. After: the open row's first sample is already its final colour, and no sidebar row runs a transition on load, on a Turbo visit or in the mobile drawer. Two transitions remain at load and neither is a sidebar row: the Mercure dot, which is neutral until the stream reports, by design, and the topbar calendar toggle, which is still rendered as "mail" and corrected on connect. SidebarRowStateTest pins the rendered state, the matching rule, and that the class lists agree with the controller's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
ca2498f6e9
commit
994d0a9c0d
6 files changed
+319
-29
No files matched your search
@@ -4,6 +4,12 @@ import { WRITE_EVENT } from "../../mail_writes.js";
|
||||
|
||||
// `is-active` carries no colour of its own — it is the hook the sidebar's
|
||||
// Gmail-style pill shape hangs off in app.css.
|
||||
//
|
||||
// The server renders these same two sets on every row it draws (OPEN and
|
||||
// CLOSED in App\Twig\SidebarRowStateExtension), so on those rows the pass in
|
||||
// connect() changes nothing. Keep the lists identical: a row the server drew
|
||||
// one way and this pass redraws the other is a row that fades on every load,
|
||||
// from whatever it was painted as. SidebarRowStateTest compares the two files.
|
||||
const ACTIVE_CLASSES = ["is-active", "bg-accent-soft", "text-accent", "font-medium"];
|
||||
// A parent of the open row. Its own class rather than the active set: the
|
||||
// trail should read as "you are in here", not as a second selection.
|
||||
@@ -667,6 +673,10 @@ export default class extends Controller {
|
||||
* compare matched nothing and those rows never highlighted at all. The
|
||||
* account has to agree too — the same label under another account is a
|
||||
* different view, and both rows lighting up says the wrong thing.
|
||||
*
|
||||
* Restated in PHP as SidebarRowStateExtension::isOpen(), which decides the
|
||||
* rows the server renders. A change here that is not made there is a row
|
||||
* the two disagree about, and it fades on every load.
|
||||
*/
|
||||
_matches(link) {
|
||||
const target = new URL(link.href, window.location.origin);
|
||||
|
||||
@@ -0,0 +1,123 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Twig;
|
||||
|
||||
use Symfony\Component\HttpFoundation\RequestStack;
|
||||
use Twig\Extension\AbstractExtension;
|
||||
use Twig\TwigFunction;
|
||||
|
||||
/**
|
||||
* Whether a sidebar row is the one you are on, decided while the page is
|
||||
* rendered rather than after it has painted.
|
||||
*
|
||||
* ui--sidebar used to be the only thing that knew. The rows went out with no
|
||||
* colour class at all, and the controller's first pass on connect gave the
|
||||
* open one `bg-accent-soft text-accent` and every other one `text-ink-muted`.
|
||||
* By then the page had been styled at least once — the first paint, or
|
||||
* anything earlier that made the browser resolve styles — and every row carries
|
||||
* `transition-colors` for its hover. So on every load, and on every Turbo
|
||||
* visit that re-rendered the sidebar, the open row faded in over 150ms from
|
||||
* the inherited black and a transparent pill, and the rest faded from black
|
||||
* to their muted ink. Measured in Chromium against the test stack: first
|
||||
* paint at ~200ms with the rows black, the pass at ~250ms, the transitions
|
||||
* starting in the frame after it.
|
||||
*
|
||||
* Rendering the classes here makes that pass a no-op on everything the server
|
||||
* drew: the first style the browser resolves is already the final one, so
|
||||
* there is nothing to transition from, and the hover transitions — declared on
|
||||
* the rows, triggered by :hover — are untouched. Same answer the More
|
||||
* disclosure's `open` and the collapsed label trees already give to the same
|
||||
* question: the server knows where you are, so it says so in the HTML.
|
||||
*
|
||||
* The controller keeps its pass, for everything the server cannot see. A
|
||||
* folder click is a FRAME navigation that re-renders no sidebar, so moving the
|
||||
* highlight there is the pass's job — a real change of place, and fading it is
|
||||
* right. Rows that arrive later — an account's folder list fetched into its
|
||||
* own frame, a label list re-rendered by a Turbo Stream — are rendered for
|
||||
* that request's URL rather than the page's, come out closed, and are marked
|
||||
* by the pass once they land. That correction still fades (measured: the open
|
||||
* folder in a freshly fetched list runs the same two transitions), which is
|
||||
* the one case this does not reach; it follows a click that changed the list,
|
||||
* never a load.
|
||||
*
|
||||
* ── The comparison is ui--sidebar#_matches, and has to stay it ─────────────
|
||||
* A row this marks open and the controller then closes (or the reverse) is
|
||||
* the fade back again, one row at a time. So the rule is the controller's,
|
||||
* restated: the path is the link's path or lies under it — the prefix is what
|
||||
* keeps a parent label open while you are in a child, and the slash is what
|
||||
* keeps "Work" from claiming "Workshop" — and a link that names an account
|
||||
* matches only that account. The class lists are the controller's too; see
|
||||
* SidebarRowStateTest, which reads both files.
|
||||
*
|
||||
* The MAIN request, because that is the one whose URL the browser is showing.
|
||||
* A stream or a frame fetch renders these rows for a URL no row points at, so
|
||||
* everything comes out closed there, which is the correct thing to hand the
|
||||
* controller.
|
||||
*/
|
||||
final class SidebarRowStateExtension extends AbstractExtension
|
||||
{
|
||||
/**
|
||||
* The open row: the filled pill. `is-active` is also the hook the pill's
|
||||
* shape and mail--dnd's "this is where you are" check hang off.
|
||||
*
|
||||
* Kept identical to ACTIVE_CLASSES in assets/controllers/ui/sidebar_controller.js.
|
||||
*/
|
||||
public const string OPEN = 'is-active bg-accent-soft text-accent font-medium';
|
||||
|
||||
/** Kept identical to INACTIVE_CLASSES in the same file. */
|
||||
public const string CLOSED = 'text-ink-muted hover:bg-hover';
|
||||
|
||||
public function __construct(private readonly RequestStack $requestStack)
|
||||
{
|
||||
}
|
||||
|
||||
public function getFunctions(): array
|
||||
{
|
||||
return [
|
||||
new TwigFunction('sidebar_row_state', $this->rowState(...)),
|
||||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* The classes for the row whose link points at $href.
|
||||
*/
|
||||
public function rowState(string $href): string
|
||||
{
|
||||
return true === $this->isOpen($href) ? self::OPEN : self::CLOSED;
|
||||
}
|
||||
|
||||
public function isOpen(string $href): bool
|
||||
{
|
||||
$request = $this->requestStack->getMainRequest();
|
||||
$target = parse_url($href);
|
||||
|
||||
if (null === $request || false === $target || false === isset($target['path'])) {
|
||||
return false;
|
||||
}
|
||||
|
||||
// What `window.location.pathname` is to the controller: base URL
|
||||
// included, and still percent-encoded — which is how path() writes the
|
||||
// href, so the two compare like for like.
|
||||
$path = $request->getBaseUrl() . $request->getPathInfo();
|
||||
|
||||
if ($path !== $target['path'] && false === str_starts_with($path, $target['path'] . '/')) {
|
||||
return false;
|
||||
}
|
||||
|
||||
parse_str($target['query'] ?? '', $query);
|
||||
|
||||
$wanted = $query['account'] ?? null;
|
||||
|
||||
if (null === $wanted) {
|
||||
return true;
|
||||
}
|
||||
|
||||
// all() rather than get(): get() throws on `?account[]=`, and a
|
||||
// malformed query string must not be able to take the sidebar down.
|
||||
$current = $request->query->all()['account'] ?? null;
|
||||
|
||||
return is_string($current) && $wanted === $current;
|
||||
}
|
||||
}
|
||||
@@ -26,13 +26,17 @@
|
||||
other sidebar row — a rounded-lg box on this wrapper was the last
|
||||
square highlight left in the sidebar. It has to be the wrapper and
|
||||
not the anchor: on the anchor the pill stops before the chevron and
|
||||
cuts the row in half. #}
|
||||
<div class="nav-item flex items-center transition-colors text-ink-muted">
|
||||
cuts the row in half.
|
||||
|
||||
Open or closed is rendered, not applied on connect — see
|
||||
App\Twig\SidebarRowStateExtension. #}
|
||||
{% set _href = path('app_mail_account', { account: account.id }) %}
|
||||
<div class="nav-item flex items-center transition-colors {{ sidebar_row_state(_href) }}">
|
||||
{# Named explicitly rather than by the address inside it: that span
|
||||
is `truncate`, and a name a screen reader reads as "kontakt@…"
|
||||
because CSS ran out of room is not a name. #}
|
||||
<a
|
||||
href="{{ path('app_mail_account', { account: account.id }) }}"
|
||||
href="{{ _href }}"
|
||||
data-ui--sidebar-target="link"
|
||||
data-action="{{ _drawerClose }}"
|
||||
aria-label="{{ 'sidebar.accounts.open'|trans({ '%account%': account.displayAddress }) }}"
|
||||
|
||||
@@ -174,6 +174,7 @@ account inside ThreadStatusUpdater.
|
||||
{% macro label_node(label, depth, drawerClose) %}
|
||||
{% import _self as macros %}
|
||||
{% set children = sidebarCounts.visibleChildrenOf(label) %}
|
||||
{% set _href = path('app_mail_label_path', { path: label.fullName }) %}
|
||||
|
||||
{% if children|length > 0 %}
|
||||
{# Keyed by full name, in the same stored set as the two section
|
||||
@@ -192,7 +193,7 @@ account inside ThreadStatusUpdater.
|
||||
data-action="toggle->ui--sidebar#rememberCollapse"
|
||||
{{ sidebarCounts.isCollapsed(_treeKey) ? '' : 'open' }}>
|
||||
<summary class="nav-item group/label flex items-center gap-1 cursor-pointer list-none
|
||||
transition-colors"
|
||||
transition-colors {{ sidebar_row_state(_href) }}"
|
||||
{# A parent label takes drops like any other. It is a real
|
||||
folder with real mail in it; having children does not
|
||||
make it a heading. #}
|
||||
@@ -204,7 +205,7 @@ account inside ThreadStatusUpdater.
|
||||
<i class="fa-solid fa-chevron-right text-[9px] text-ink-faint w-3
|
||||
transition-transform group-open/tree:rotate-90" aria-hidden="true"></i>
|
||||
{{ macros.label_dot(label.color) }}
|
||||
<a href="{{ path('app_mail_label_path', { path: label.fullName }) }}"
|
||||
<a href="{{ _href }}"
|
||||
{{ macros.list_frame() }}
|
||||
class="flex-1 flex items-center justify-between gap-2 truncate"
|
||||
data-ui--sidebar-target="link"
|
||||
@@ -231,12 +232,12 @@ account inside ThreadStatusUpdater.
|
||||
</div>
|
||||
</details>
|
||||
{% else %}
|
||||
<div class="nav-item group/label flex items-center gap-2 transition-colors"
|
||||
<div class="nav-item group/label flex items-center gap-2 transition-colors {{ sidebar_row_state(_href) }}"
|
||||
{{ macros.drop_target(label.id) }}
|
||||
data-nav-tier="tree"
|
||||
style="--label-depth: {{ depth * 14 + 16 }}px">
|
||||
{{ macros.label_dot(label.color) }}
|
||||
<a href="{{ path('app_mail_label_path', { path: label.fullName }) }}"
|
||||
<a href="{{ _href }}"
|
||||
{{ macros.list_frame() }}
|
||||
class="flex-1 flex items-center justify-between gap-2 truncate min-w-0"
|
||||
data-ui--sidebar-target="link"
|
||||
@@ -398,12 +399,21 @@ controller instead — the scroll position and which trees are open.
|
||||
data-ui--sidebar-target="scroller"
|
||||
data-action="scroll->ui--sidebar#rememberScroll">
|
||||
|
||||
{# Which row is open is RENDERED, by sidebar_row_state() on every row
|
||||
below, not left to ui--sidebar's pass on connect: arriving after the
|
||||
page had been styled, that pass turned into a 150ms fade from black
|
||||
on every row, on every load. The href is set once per row because the state
|
||||
is decided from it — a row whose link and state were written out
|
||||
separately could point at one place and light up for another. See
|
||||
App\Twig\SidebarRowStateExtension. #}
|
||||
|
||||
{# Every system row answers to its eye toggle in label settings. Only
|
||||
Spam used to: the toggle saved the flag for Inbox or Trash, the
|
||||
settings list greyed the label out, and the row stayed. A role with
|
||||
no label yet counts as shown — see SidebarCounts::isRoleHidden(). #}
|
||||
{% if not sidebarCounts.isRoleHidden(enum('App\\Domain\\Enum\\Mail\\LabelRole').Inbox) %}
|
||||
<div class="nav-item flex items-center justify-between gap-2 px-3 rounded-lg transition-colors"
|
||||
{% set _href = path('app_mail_inbox') %}
|
||||
<div class="nav-item flex items-center justify-between gap-2 px-3 rounded-lg transition-colors {{ sidebar_row_state(_href) }}"
|
||||
{# Dropping a conversation here is the same operation as the row's
|
||||
own "back to the inbox" button: out of the bin, out of spam, out
|
||||
of the archive. See ThreadStatusUpdater::move(), which sends
|
||||
@@ -417,7 +427,7 @@ controller instead — the scroll position and which trees are open.
|
||||
the same to the pixel as when the anchor WAS the row. #}
|
||||
<a class="flex-1 min-w-0 flex items-center gap-2"
|
||||
data-nav-tier="row"
|
||||
href="{{ path('app_mail_inbox') }}"
|
||||
href="{{ _href }}"
|
||||
{{ macros.list_frame() }}
|
||||
data-ui--sidebar-target="link"
|
||||
data-action="{{ _drawerClose }}"
|
||||
@@ -440,7 +450,8 @@ controller instead — the scroll position and which trees are open.
|
||||
a state, not a folder, so data-dnd-unread rather than drop_target(),
|
||||
which only knows how to move or to label. See mail--dnd. #}
|
||||
{% if sidebarCounts.hasVisibleRole(enum('App\\Domain\\Enum\\Mail\\LabelRole').Unread) %}
|
||||
<div class="nav-item flex items-center justify-between gap-2 px-3 rounded-lg transition-colors"
|
||||
{% set _href = path('app_mail_unread') %}
|
||||
<div class="nav-item flex items-center justify-between gap-2 px-3 rounded-lg transition-colors {{ sidebar_row_state(_href) }}"
|
||||
data-dnd-unread>
|
||||
{# The tier rides on the LINK, not on the pill around it. The pill is
|
||||
what you see — .nav-item draws it — but the row's height comes from
|
||||
@@ -450,7 +461,7 @@ controller instead — the scroll position and which trees are open.
|
||||
the same to the pixel as when the anchor WAS the row. #}
|
||||
<a class="flex-1 min-w-0 flex items-center gap-2"
|
||||
data-nav-tier="row"
|
||||
href="{{ path('app_mail_unread') }}"
|
||||
href="{{ _href }}"
|
||||
{{ macros.list_frame() }}
|
||||
data-ui--sidebar-target="link"
|
||||
data-action="{{ _drawerClose }}"
|
||||
@@ -467,7 +478,8 @@ controller instead — the scroll position and which trees are open.
|
||||
|
||||
{# A drop here stars the mail and leaves it where it is, as the Unread
|
||||
row above marks it unread: a state, not a folder. See mail--dnd. #}
|
||||
<div class="nav-item flex items-center justify-between gap-2 px-3 rounded-lg transition-colors"
|
||||
{% set _href = path('app_mail_starred') %}
|
||||
<div class="nav-item flex items-center justify-between gap-2 px-3 rounded-lg transition-colors {{ sidebar_row_state(_href) }}"
|
||||
data-dnd-star>
|
||||
{# The tier rides on the LINK, not on the pill around it. The pill is
|
||||
what you see — .nav-item draws it — but the row's height comes from
|
||||
@@ -477,7 +489,7 @@ controller instead — the scroll position and which trees are open.
|
||||
the same to the pixel as when the anchor WAS the row. #}
|
||||
<a class="flex-1 min-w-0 flex items-center gap-2"
|
||||
data-nav-tier="row"
|
||||
href="{{ path('app_mail_starred') }}"
|
||||
href="{{ _href }}"
|
||||
{{ macros.list_frame() }}
|
||||
data-ui--sidebar-target="link"
|
||||
data-action="{{ _drawerClose }}"
|
||||
@@ -493,9 +505,10 @@ controller instead — the scroll position and which trees are open.
|
||||
</div>
|
||||
|
||||
{% if not sidebarCounts.isRoleHidden(enum('App\\Domain\\Enum\\Mail\\LabelRole').Sent) %}
|
||||
<a href="{{ path('app_mail_sent') }}"
|
||||
{% set _href = path('app_mail_sent') %}
|
||||
<a href="{{ _href }}"
|
||||
{{ macros.list_frame() }}
|
||||
class="nav-item flex items-center justify-between gap-2 px-3 rounded-lg transition-colors"
|
||||
class="nav-item flex items-center justify-between gap-2 px-3 rounded-lg transition-colors {{ sidebar_row_state(_href) }}"
|
||||
data-nav-tier="row"
|
||||
data-ui--sidebar-target="link"
|
||||
data-action="{{ _drawerClose }}"
|
||||
@@ -511,9 +524,10 @@ controller instead — the scroll position and which trees are open.
|
||||
{% endif %}
|
||||
|
||||
{% if not sidebarCounts.isRoleHidden(enum('App\\Domain\\Enum\\Mail\\LabelRole').Drafts) %}
|
||||
<a href="{{ path('app_mail_drafts') }}"
|
||||
{% set _href = path('app_mail_drafts') %}
|
||||
<a href="{{ _href }}"
|
||||
{{ macros.list_frame() }}
|
||||
class="nav-item flex items-center justify-between gap-2 px-3 rounded-lg transition-colors"
|
||||
class="nav-item flex items-center justify-between gap-2 px-3 rounded-lg transition-colors {{ sidebar_row_state(_href) }}"
|
||||
data-nav-tier="row"
|
||||
data-ui--sidebar-target="link"
|
||||
data-action="{{ _drawerClose }}"
|
||||
@@ -534,8 +548,9 @@ controller instead — the scroll position and which trees are open.
|
||||
topbar there instead. Below lg there is nowhere to dock it, so this
|
||||
is a plain link to the page like any other row here. #}
|
||||
{% if _drawerMode is defined and _drawerMode %}
|
||||
<a href="{{ path('app_calendar_index') }}"
|
||||
class="nav-item flex items-center justify-between gap-2 px-3 rounded-lg transition-colors"
|
||||
{% set _href = path('app_calendar_index') %}
|
||||
<a href="{{ _href }}"
|
||||
class="nav-item flex items-center justify-between gap-2 px-3 rounded-lg transition-colors {{ sidebar_row_state(_href) }}"
|
||||
data-nav-tier="row"
|
||||
data-ui--sidebar-target="link"
|
||||
data-action="{{ _drawerClose }}"
|
||||
@@ -586,7 +601,8 @@ controller instead — the scroll position and which trees are open.
|
||||
<span>{{ 'sidebar.nav.more'|trans }}</span>
|
||||
</summary>
|
||||
{% if _showSnoozed %}
|
||||
<div class="nav-item flex items-center justify-between gap-2 pl-9 pr-3 rounded-lg transition-colors">
|
||||
{% set _href = path('app_mail_snoozed') %}
|
||||
<div class="nav-item flex items-center justify-between gap-2 pl-9 pr-3 rounded-lg transition-colors {{ sidebar_row_state(_href) }}">
|
||||
{# The tier rides on the LINK, not on the pill around it. The pill is
|
||||
what you see — .nav-item draws it — but the row's height comes from
|
||||
padding-block, and padding on the wrapper is area the anchor does not
|
||||
@@ -595,7 +611,7 @@ controller instead — the scroll position and which trees are open.
|
||||
the same to the pixel as when the anchor WAS the row. #}
|
||||
<a class="flex-1 min-w-0 flex items-center gap-2"
|
||||
data-nav-tier="row"
|
||||
href="{{ path('app_mail_snoozed') }}"
|
||||
href="{{ _href }}"
|
||||
{{ macros.list_frame() }}
|
||||
data-ui--sidebar-target="link"
|
||||
data-action="{{ _drawerClose }}"
|
||||
@@ -611,7 +627,8 @@ controller instead — the scroll position and which trees are open.
|
||||
</div>
|
||||
{% endif %}
|
||||
{% if _showArchive %}
|
||||
<div class="nav-item flex items-center justify-between gap-2 pl-9 pr-3 rounded-lg transition-colors"
|
||||
{% set _href = path('app_mail_archive') %}
|
||||
<div class="nav-item flex items-center justify-between gap-2 pl-9 pr-3 rounded-lg transition-colors {{ sidebar_row_state(_href) }}"
|
||||
{{ macros.drop_target(sidebarCounts.roleLabelId(enum('App\\Domain\\Enum\\Mail\\LabelRole').Archive), null, true) }}>
|
||||
{# The tier rides on the LINK, not on the pill around it. The pill is
|
||||
what you see — .nav-item draws it — but the row's height comes from
|
||||
@@ -621,7 +638,7 @@ controller instead — the scroll position and which trees are open.
|
||||
the same to the pixel as when the anchor WAS the row. #}
|
||||
<a class="flex-1 min-w-0 flex items-center gap-2"
|
||||
data-nav-tier="row"
|
||||
href="{{ path('app_mail_archive') }}"
|
||||
href="{{ _href }}"
|
||||
{{ macros.list_frame() }}
|
||||
data-ui--sidebar-target="link"
|
||||
data-action="{{ _drawerClose }}"
|
||||
@@ -644,7 +661,8 @@ controller instead — the scroll position and which trees are open.
|
||||
visible in settings did nothing observable anywhere. Sits above
|
||||
Trash because that is where LabelRole::sortOrder() already put it. #}
|
||||
{% if _showSpam %}
|
||||
<div class="nav-item flex items-center justify-between gap-2 pl-9 pr-3 rounded-lg transition-colors"
|
||||
{% set _href = path('app_mail_spam') %}
|
||||
<div class="nav-item flex items-center justify-between gap-2 pl-9 pr-3 rounded-lg transition-colors {{ sidebar_row_state(_href) }}"
|
||||
{# "This is junk", said by dropping. The only way to say it in
|
||||
the interface: there is no mark-as-spam button anywhere, and
|
||||
the row is here because the folder is. #}
|
||||
@@ -657,7 +675,7 @@ controller instead — the scroll position and which trees are open.
|
||||
the same to the pixel as when the anchor WAS the row. #}
|
||||
<a class="flex-1 min-w-0 flex items-center gap-2"
|
||||
data-nav-tier="row"
|
||||
href="{{ path('app_mail_spam') }}"
|
||||
href="{{ _href }}"
|
||||
{{ macros.list_frame() }}
|
||||
data-ui--sidebar-target="link"
|
||||
data-action="{{ _drawerClose }}"
|
||||
@@ -674,9 +692,10 @@ controller instead — the scroll position and which trees are open.
|
||||
{% endif %}
|
||||
|
||||
{% if _showTrash %}
|
||||
<a href="{{ path('app_mail_trash') }}"
|
||||
{% set _href = path('app_mail_trash') %}
|
||||
<a href="{{ _href }}"
|
||||
{{ macros.list_frame() }}
|
||||
class="nav-item flex items-center justify-between gap-2 pl-9 pr-3 rounded-lg transition-colors"
|
||||
class="nav-item flex items-center justify-between gap-2 pl-9 pr-3 rounded-lg transition-colors {{ sidebar_row_state(_href) }}"
|
||||
{{ macros.drop_target(sidebarCounts.roleLabelId(enum('App\\Domain\\Enum\\Mail\\LabelRole').Trash), null, true) }}
|
||||
data-nav-tier="row"
|
||||
data-ui--sidebar-target="link"
|
||||
|
||||
@@ -54,13 +54,19 @@
|
||||
the drop_target macro. #}
|
||||
{% import '_partials/_sidebar.html.twig' as sidebarMacros %}
|
||||
{% set droppable = label.role is null or label.role.acceptsMoves %}
|
||||
<div class="nav-item flex items-center justify-between text-xs transition-colors text-ink-muted"
|
||||
{# Open or closed is rendered, not applied on connect — see
|
||||
App\Twig\SidebarRowStateExtension. Only on the sidebar's own render
|
||||
can it be right: fetched into the frame, these rows are rendered for
|
||||
the fetch's URL, come out closed, and ui--sidebar marks the open one
|
||||
as they arrive. #}
|
||||
{% set _href = path('app_mail_label', { id: label.id, account: account.id }) %}
|
||||
<div class="nav-item flex items-center justify-between text-xs transition-colors {{ sidebar_row_state(_href) }}"
|
||||
{% if droppable %}{{ sidebarMacros.drop_target(label.id, account, true) }}{% endif %}
|
||||
data-nav-tier="tree"
|
||||
style="--label-depth: {{ 12 + label.depth * 12 }}px">
|
||||
<a
|
||||
data-ui--sidebar-target="link"
|
||||
href="{{ path('app_mail_label', { id: label.id, account: account.id }) }}"
|
||||
href="{{ _href }}"
|
||||
{# BOTH of these, and the pair is the point.
|
||||
|
||||
`_top` is the answer that is correct on every page. These
|
||||
|
||||
@@ -0,0 +1,128 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Tests\Controller;
|
||||
|
||||
use App\Entity\User\User;
|
||||
use App\Repository\User\UserRepository;
|
||||
use App\Twig\SidebarRowStateExtension;
|
||||
use PHPUnit\Framework\Attributes\DataProvider;
|
||||
use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
|
||||
use Symfony\Component\DomCrawler\Crawler;
|
||||
use Symfony\Component\HttpFoundation\Request;
|
||||
use Symfony\Component\HttpFoundation\RequestStack;
|
||||
|
||||
/**
|
||||
* Which sidebar row is open is in the HTML, not applied after paint.
|
||||
*
|
||||
* The rows used to go out with no colour class, and ui--sidebar gave the open
|
||||
* one its pill on connect — after the browser had already styled the page, so
|
||||
* every row's hover transition turned that pass into a 150ms fade from black,
|
||||
* on every load and every Turbo visit. SidebarRowStateExtension renders the
|
||||
* classes instead, and the controller's pass then changes nothing.
|
||||
*
|
||||
* Asserted against the raw response, like SidebarSectionCollapseTest: the test
|
||||
* client runs no JavaScript, so a class seen here can only have come from the
|
||||
* server. The browser-level reading — `transitionrun` events and per-frame
|
||||
* computed colours through a load — was taken in Chromium when this changed;
|
||||
* it is not repeated here.
|
||||
*
|
||||
* The fix holds only while the server and the controller agree, on the rule
|
||||
* and on the classes, so both halves of that agreement are pinned below.
|
||||
*/
|
||||
final class SidebarRowStateTest extends WebTestCase
|
||||
{
|
||||
private const string CONTROLLER = 'assets/controllers/ui/sidebar_controller.js';
|
||||
|
||||
public function testTheOpenRowIsRenderedAndNoRowIsLeftForTheControllerToColour(): void
|
||||
{
|
||||
$client = static::createClient();
|
||||
$user = static::getContainer()->get(UserRepository::class)
|
||||
->findOneBy(['email' => 'e2e-admin@plmail.test']);
|
||||
|
||||
if (false === $user instanceof User) {
|
||||
self::markTestSkipped('run `app:test:seed-user --admin` first');
|
||||
}
|
||||
|
||||
$client->loginUser($user);
|
||||
$crawler = $client->request('GET', '/mail/inbox');
|
||||
|
||||
$rows = $crawler
|
||||
->filter('#sidebar [data-ui--sidebar-target="link"], #sidebar-drawer-inner [data-ui--sidebar-target="link"]')
|
||||
->each(static function (Crawler $link): array {
|
||||
$row = $link->closest('.nav-item') ?? $link;
|
||||
|
||||
return [(string) $link->attr('href'), explode(' ', (string) $row->attr('class'))];
|
||||
});
|
||||
|
||||
self::assertNotEmpty($rows, 'no sidebar rows rendered at all');
|
||||
|
||||
$open = [];
|
||||
|
||||
foreach ($rows as [$href, $classes]) {
|
||||
$isOpen = [] === array_diff(explode(' ', SidebarRowStateExtension::OPEN), $classes);
|
||||
$isClosed = [] === array_diff(explode(' ', SidebarRowStateExtension::CLOSED), $classes);
|
||||
|
||||
// Neither is the regression: a row the controller colours on
|
||||
// connect, from whatever the browser had already painted it as.
|
||||
self::assertTrue(
|
||||
$isOpen xor $isClosed,
|
||||
sprintf('the row for %s was rendered without its open/closed state: %s', $href, implode(' ', $classes)),
|
||||
);
|
||||
|
||||
if ($isOpen) {
|
||||
$open[] = $href;
|
||||
}
|
||||
}
|
||||
|
||||
// Once in the sidebar and once in the drawer, which is the same partial.
|
||||
self::assertSame(['/mail/inbox', '/mail/inbox'], $open);
|
||||
}
|
||||
|
||||
/**
|
||||
* ui--sidebar#_matches, restated — the cases where getting it slightly
|
||||
* wrong would be invisible until a row faded.
|
||||
*
|
||||
* @return iterable<string, array{string, string, bool}>
|
||||
*/
|
||||
public static function rows(): iterable
|
||||
{
|
||||
yield 'a parent label is open while a child is' => ['/mail/label/path/Work/Clients', '/mail/label/path/Work', true];
|
||||
yield 'a label sharing a prefix is not its child' => ['/mail/label/path/Workshop', '/mail/label/path/Work', false];
|
||||
yield 'an account folder, on that account' => ['/mail/label/5?account=3', '/mail/label/5?account=3', true];
|
||||
yield 'an account folder, on another account' => ['/mail/label/5?account=4', '/mail/label/5?account=3', false];
|
||||
yield 'a malformed account is closed, not an error' => ['/mail/label/5?account[]=3', '/mail/label/5?account=3', false];
|
||||
}
|
||||
|
||||
#[DataProvider('rows')]
|
||||
public function testARowIsOpenExactlyWhereTheControllerWouldOpenIt(string $page, string $href, bool $open): void
|
||||
{
|
||||
$stack = new RequestStack();
|
||||
$stack->push(Request::create($page));
|
||||
|
||||
self::assertSame($open, new SidebarRowStateExtension($stack)->isOpen($href));
|
||||
}
|
||||
|
||||
public function testTheServerAndTheControllerDrawARowTheSameWay(): void
|
||||
{
|
||||
$source = (string) file_get_contents(dirname(__DIR__, 2) . '/' . self::CONTROLLER);
|
||||
|
||||
foreach (['ACTIVE_CLASSES' => SidebarRowStateExtension::OPEN, 'INACTIVE_CLASSES' => SidebarRowStateExtension::CLOSED] as $constant => $rendered) {
|
||||
self::assertSame(1, preg_match('/const ' . $constant . '\s*=\s*\[([^\]]*)\]/', $source, $match), sprintf('%s no longer declares %s', self::CONTROLLER, $constant));
|
||||
|
||||
preg_match_all('/"([^"]+)"/', $match[1], $classes);
|
||||
|
||||
self::assertSame(
|
||||
explode(' ', $rendered),
|
||||
$classes[1],
|
||||
sprintf(
|
||||
'%s in %s and the classes SidebarRowStateExtension renders have drifted apart. '
|
||||
. 'Every row the server draws would then be redrawn on connect — and fade.',
|
||||
$constant,
|
||||
self::CONTROLLER,
|
||||
),
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user