fix: MPS-24 — facets compose; sort+price survive switching category
Operator: 'switching one breaks it' — picking a category reset the
active Sort + Price. Cause: facet category links / 'All' link / top
chips / lane 'See all' all pointed at a bare {tag_base}/tag/{slug}
with NO query string, so a click dropped ?sort= / ?price_*. (The
Sort select / Price form already preserved the tag via action='' +
path and each other as sibling fields — only category nav lost state.)
Fix: one shared facet_qs(sort_key, price_min, price_max) macro in
_facet_nav.j2 returning the ?sort=...&price_min=...&price_max=...
suffix, appended to every category/All/chip/See-all href in
_facet_nav.j2, home.j2, shop.j2. URL state, NOT localStorage
(operator's suggestion): shareable, no-JS, back-button correct, and
the destination SERP already reads those params. The & is HTML-escaped
to & in hrefs (Jinja autoescape) — browsers decode it fine.
Tests: +test_facet_links_preserve_sort_and_price; updated
test_tag_detail_renders_facet_sidebar +
test_facet_category_link_renders_tag_detail_not_home for the new
(correct) query-carrying behavior. 1135 passed.
Docs: mps-24.md Phases 2.8e–2.8h.
This commit is contained in:
parent
bcd8c2471b
commit
3bb05e4b5d
5 changed files with 107 additions and 11 deletions
|
|
@ -363,7 +363,40 @@ Tests (`test_functional.py::TestProductTagsSpa`):
|
|||
Deferred (occasional click, not the hot path): AJAX-ifying the
|
||||
"Suggest categories" link — still a full navigation by design.
|
||||
|
||||
**Phase 2.8e — the LAST link: `form.action` DOM-clobbered** (shipped
|
||||
**Phase 2.8h — facets compose, not clobber** (shipped 2026-05-16):
|
||||
operator: "switching one breaks it" — picking a category reset the
|
||||
active Sort + Price. Cause: the facet category links / "All" link /
|
||||
top chips / lane "See all" all pointed at a bare
|
||||
`{tag_base}/tag/{slug}` with **no query string**, so a click dropped
|
||||
`?sort=` / `?price_*`. (The Sort `<select>`/Price form already
|
||||
preserved the tag via `action=""` + path, and each other as sibling
|
||||
fields — only category navigation lost state.) Fix: one shared
|
||||
`facet_qs(sort_key, price_min, price_max)` macro in `_facet_nav.j2`
|
||||
returning the `?sort=…&price_min=…&price_max=…` suffix; appended to
|
||||
every category/All/chip/See-all href in `_facet_nav.j2`, `home.j2`,
|
||||
`shop.j2`. URL state, **not localStorage** (operator's suggestion):
|
||||
shareable, no-JS, back-button correct, and the destination SERP
|
||||
already reads those params. Tests:
|
||||
`test_facet_links_preserve_sort_and_price`,
|
||||
updated `test_tag_detail_renders_facet_sidebar` /
|
||||
`test_facet_category_link_renders_tag_detail_not_home`.
|
||||
|
||||
**Phase 2.8g — facet sidebar nested scrollbar removed** (shipped
|
||||
2026-05-16): `ul.facet-tag-list` had `max-height:60vh;
|
||||
overflow-y:auto` → ugly inner scrollbar on the sidebar / mobile
|
||||
accordion. Dropped; the list flows full-height and the page scrolls.
|
||||
|
||||
**Phase 2.8f — chips navigate to the tag SERP like the left nav**
|
||||
(shipped 2026-05-16): the top chip strip's category links already
|
||||
pointed at `{tag_base}/tag/{slug}`, but `tag_filter.js` decided
|
||||
whether to intercept by inspecting **`chips[0]`** — the "All" chip,
|
||||
which points at the shop home (no `/tag/`) — so it never detected the
|
||||
real category links and always did the in-place "default cards"
|
||||
hide/show. Now it scans ALL chips: any `/tag/` href → bail → full
|
||||
navigation, so a chip behaves exactly like its matching left-nav
|
||||
category (server renders the SERP in the shop's `home_layout`).
|
||||
|
||||
**Phase 2.8e — `form.action` DOM-clobbered by `<input name=action>`** (shipped
|
||||
2026-05-16): with 2.8d live, the operator's Network panel showed the
|
||||
proxy-proof `ajax=1` working (a real `fetch` to `tags` → 200, 0.7 kB
|
||||
JSON) — but also four requests to a URL literally named
|
||||
|
|
|
|||
|
|
@ -29,6 +29,20 @@
|
|||
{%- if cents is not none -%}{{ (cents / 100)|round(2) }}{%- endif -%}
|
||||
{% endmacro %}
|
||||
|
||||
{# Carry the active sort + price across every category link / chip so
|
||||
switching category does NOT reset sort/price (and vice versa). URL
|
||||
state, not localStorage: shareable, no-JS, back-button correct, and
|
||||
the destination SERP already reads ?sort= / ?price_*. Returns the
|
||||
FULL suffix incl. leading "?" when non-empty, else "" — append it
|
||||
verbatim to a query-less href. #}
|
||||
{% macro facet_qs(sort_key, price_min, price_max) -%}
|
||||
{%- set ns = namespace(parts=[]) -%}
|
||||
{%- if sort_key -%}{%- set ns.parts = ns.parts + ['sort=' ~ sort_key] -%}{%- endif -%}
|
||||
{%- if price_min is not none -%}{%- set ns.parts = ns.parts + ['price_min=' ~ (fmt_cents(price_min)|trim)] -%}{%- endif -%}
|
||||
{%- if price_max is not none -%}{%- set ns.parts = ns.parts + ['price_max=' ~ (fmt_cents(price_max)|trim)] -%}{%- endif -%}
|
||||
{%- if ns.parts -%}?{{ ns.parts|join('&') }}{%- endif -%}
|
||||
{%- endmacro %}
|
||||
|
||||
{% macro facet_form(base_url, tag_base, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) %}
|
||||
<form method="get" action="" class="facet-form">
|
||||
|
||||
|
|
@ -68,13 +82,13 @@
|
|||
<h2 class="facet-title">Categories</h2>
|
||||
<ul class="facet-tag-list">
|
||||
<li>
|
||||
<a href="{{ base_url }}"
|
||||
<a href="{{ base_url }}{{ facet_qs(sort_key, price_min, price_max) }}"
|
||||
class="facet-tag{% if active_tag is none %} facet-tag-active{% endif %}"
|
||||
rel="nofollow">All</a>
|
||||
</li>
|
||||
{% for t in facet_tags %}
|
||||
<li>
|
||||
<a href="{{ tag_base }}/tag/{{ t.slug }}"
|
||||
<a href="{{ tag_base }}/tag/{{ t.slug }}{{ facet_qs(sort_key, price_min, price_max) }}"
|
||||
class="facet-tag{% if active_tag and active_tag.id == t.id %} facet-tag-active{% endif %}"
|
||||
rel="nofollow">
|
||||
<span class="facet-tag-name">{{ t.name }}</span>
|
||||
|
|
|
|||
|
|
@ -94,12 +94,12 @@
|
|||
lock-step. tag_filter.js keeps full navigation for /tag/ hrefs. #}
|
||||
{% if home_chips %}
|
||||
<nav class="tag-chip-strip" data-tag-strip aria-label="Browse by category">
|
||||
<a href="{{ shop_url }}"
|
||||
<a href="{{ shop_url }}{{ facet.facet_qs(sort_key, price_min, price_max) }}"
|
||||
class="tag-chip{% if not active_tag %} tag-chip-active{% endif %}"
|
||||
data-tag-slug=""
|
||||
rel="nofollow">All</a>
|
||||
{% for chip in home_chips %}
|
||||
<a href="{{ tag_base }}/tag/{{ chip.slug }}"
|
||||
<a href="{{ tag_base }}/tag/{{ chip.slug }}{{ facet.facet_qs(sort_key, price_min, price_max) }}"
|
||||
class="tag-chip{% if active_tag and active_tag.id == chip.id %} tag-chip-active{% endif %}"
|
||||
data-tag-slug="{{ chip.slug }}"
|
||||
rel="nofollow">{{ chip.name }}</a>
|
||||
|
|
@ -124,7 +124,7 @@
|
|||
<section class="tag-lane" data-tag-lane="{{ lane.tag.slug }}">
|
||||
<header class="tag-lane-header">
|
||||
<h2 class="type-title tag-lane-title">{{ lane.tag.name }}</h2>
|
||||
<a href="{{ tag_base }}/tag/{{ lane.tag.slug }}" class="tag-lane-more shop-theme-link-color" rel="nofollow">See all →</a>
|
||||
<a href="{{ tag_base }}/tag/{{ lane.tag.slug }}{{ facet.facet_qs(sort_key, price_min, price_max) }}" class="tag-lane-more shop-theme-link-color" rel="nofollow">See all →</a>
|
||||
</header>
|
||||
|
||||
{# Desktop: horizontal tile scroll (Netflix-style). Hidden <800px. #}
|
||||
|
|
|
|||
|
|
@ -24,12 +24,12 @@
|
|||
state. #}
|
||||
{% if home_chips %}
|
||||
<nav class="tag-chip-strip" data-tag-strip aria-label="Browse by category">
|
||||
<a href="{{ shop_url }}"
|
||||
<a href="{{ shop_url }}{{ facet.facet_qs(sort_key, price_min, price_max) }}"
|
||||
class="tag-chip{% if not active_tag %} tag-chip-active{% endif %}"
|
||||
data-tag-slug=""
|
||||
rel="nofollow">All</a>
|
||||
{% for chip in home_chips %}
|
||||
<a href="{{ tag_base }}/tag/{{ chip.slug }}"
|
||||
<a href="{{ tag_base }}/tag/{{ chip.slug }}{{ facet.facet_qs(sort_key, price_min, price_max) }}"
|
||||
class="tag-chip{% if active_tag and active_tag.id == chip.id %} tag-chip-active{% endif %}"
|
||||
data-tag-slug="{{ chip.slug }}"
|
||||
rel="nofollow">{{ chip.name }}</a>
|
||||
|
|
@ -53,7 +53,7 @@
|
|||
<section class="tag-lane" data-tag-lane="{{ lane.tag.slug }}">
|
||||
<header class="tag-lane-header">
|
||||
<h2 class="type-title tag-lane-title">{{ lane.tag.name }}</h2>
|
||||
<a href="{{ tag_base }}/tag/{{ lane.tag.slug }}" class="tag-lane-more shop-theme-link-color" rel="nofollow">See all →</a>
|
||||
<a href="{{ tag_base }}/tag/{{ lane.tag.slug }}{{ facet.facet_qs(sort_key, price_min, price_max) }}" class="tag-lane-more shop-theme-link-color" rel="nofollow">See all →</a>
|
||||
</header>
|
||||
|
||||
{# Desktop: horizontal tile scroll. Hidden <800px. #}
|
||||
|
|
|
|||
|
|
@ -8903,7 +8903,10 @@ class TestHomeLayoutAndTags(_AuthenticatedBase):
|
|||
# (/s/{id}/tag/{slug}), NOT /s/{id}/{shop_slug}/tag/{slug}.
|
||||
# The latter falls through to the shop_slug catch-all and
|
||||
# renders the shop home instead of the filtered tag page.
|
||||
self.assertIn(f'/s/{shop.id}/tag/math"', body)
|
||||
# The link now also carries the active sort/price as a query
|
||||
# string so switching category doesn't reset them (facet_qs).
|
||||
self.assertIn(f'/s/{shop.id}/tag/math?', body)
|
||||
self.assertIn(f'/s/{shop.id}/tag/math?sort=', body)
|
||||
self.assertNotIn(f'/s/{shop.id}/{shop.slug}/tag/', body)
|
||||
|
||||
def test_facet_category_link_renders_tag_detail_not_home(self):
|
||||
|
|
@ -8933,7 +8936,9 @@ class TestHomeLayoutAndTags(_AuthenticatedBase):
|
|||
)
|
||||
# The shop home sidebar link must be the slug-less tag route.
|
||||
res = self.testapp.get(f"/s/{shop.id}/{shop.slug}")
|
||||
self.assertIn(f'/s/{shop.id}/tag/math"', res.body.decode())
|
||||
# Slug-less tag route, now carrying the facet query string so
|
||||
# switching category preserves sort/price (facet_qs).
|
||||
self.assertIn(f'/s/{shop.id}/tag/math?', res.body.decode())
|
||||
# Following that exact route renders the tag detail SERP page,
|
||||
# which carries the unique .tag-detail-header h1 (the shop home
|
||||
# lanes view never emits that element).
|
||||
|
|
@ -8942,6 +8947,50 @@ class TestHomeLayoutAndTags(_AuthenticatedBase):
|
|||
self.assertIn('tag-detail-header', body)
|
||||
self.assertIn('serp-list', body)
|
||||
|
||||
def test_facet_links_preserve_sort_and_price(self):
|
||||
"""Operator report: 'switching one breaks it' — changing category
|
||||
reset sort/price. Every category link / All link / chip / lane
|
||||
'See all' must carry the active sort+price (facet_qs) so the
|
||||
facets compose instead of clobbering each other."""
|
||||
shop, product = self._make_shop_with_product("facet-state-shop")
|
||||
self.testapp.post(
|
||||
f"/s/{shop.id}/settings",
|
||||
{
|
||||
"form_section": "home-layout-settings",
|
||||
"home_layout": "2",
|
||||
"home_layout_tag_limit": "5",
|
||||
"home_layout_per_lane_limit": "10",
|
||||
"submit": "Save Home Layout",
|
||||
},
|
||||
)
|
||||
self.testapp.post(
|
||||
f"/p/{product.id}/edit",
|
||||
{
|
||||
"title": product.title,
|
||||
"description": "One. Two. Three. Four. Five. Six.",
|
||||
"price": str(product.price),
|
||||
"visibility": "1",
|
||||
"tags": "Math",
|
||||
},
|
||||
)
|
||||
# Land on the tag SERP with an active sort + price floor.
|
||||
res = self.testapp.get(
|
||||
f"/s/{shop.id}/tag/math?sort=price_asc&price_min=1"
|
||||
)
|
||||
body = res.body.decode()
|
||||
# The "All" link must carry sort+price (so clearing the category
|
||||
# keeps the shopper's sort/price), and so must other category
|
||||
# links — none may drop the state.
|
||||
self.assertIn("sort=price_asc", body)
|
||||
self.assertIn("price_min=1", body)
|
||||
# No category link may point at the bare slug-less route with no
|
||||
# query string (that would reset sort/price on click).
|
||||
self.assertNotIn(f'/s/{shop.id}/tag/math"', body)
|
||||
# Both facets travel together on the SAME link (compose, not
|
||||
# clobber). The & is HTML-escaped to & in the href attribute
|
||||
# (Jinja autoescape) — browsers decode it back to & in the URL.
|
||||
self.assertIn("sort=price_asc&price_min=1.0", body)
|
||||
|
||||
def test_tag_detail_price_filter_narrows_grid(self):
|
||||
"""?price_min and ?price_max remove products outside the range."""
|
||||
shop = self._create_shop_helper(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue