[UI] Redesign the homepage around search, stats and a lighter hero - #1823
[UI] Redesign the homepage around search, stats and a lighter hero#1823EdouardCourty wants to merge 2 commits into
Conversation
70ba8d8 to
ac6858a
Compare
|
Going to post Claude review, but I haven't checked all findings yet so maybe don't act on it yet, I can also take over maybe because I think a bunch of stuff need "business" decisions and not just following blindly what it is blabbering :) |
Seldaek
left a comment
There was a problem hiding this comment.
Deep review of ac6858a1
I read the complete post-PR state of every changed file plus layout.html.twig, js/search.js,
BlogRssFetcher, the Doctrine/cache/Twig/CSP configs, translations/messages.en.yml and the
PHPStan config. No crash-level bugs. The direction is good — the hero reads far better than
the old two-column wall of docs. What's left is robustness, layout correctness and a couple of
content decisions.
Most of the small stuff is left as inline suggestions you can commit directly. The items below
either span several files or need a decision.
Should fix before merge
1. / should degrade, not 500, when the DB or Redis is unavailable
This is the site's entry point and the most CDN-cached page we have, and the PR gives it two
new hard backend dependencies:
- The Redis catch only handles
Predis\Connection\ConnectionException. Predis raises
Predis\Response\ServerExceptionfor-LOADING,-MISCONF,-OOMand-READONLY, and
the two are siblings, not parent/child —ConnectionException→CommunicationException→
PredisException, whileServerExceptionextendsPredisExceptiondirectly. So a Redis
restart or failover (LOADING Redis is loading the dataset in memory) escapes the catch.
\Predis\PredisExceptioncovers both. - The two
getTotal()calls have no error handling at all. They go through
Connection::executeCacheQuery()with a 24h profile, so the steady state is cheap — but on a
result-cache miss or eviction they hit MySQL, and any DB hiccup then 500s the homepage.
\Doctrine\DBAL\Exceptionis an interface in DBAL 4.x implemented byDriverException→
ConnectionException/ServerException/ConnectionLost, so one catch clause covers
connect failures, lost connections and server errors.
Worth doing because the fallback genuinely works for anonymous visitors: checkForQueryMatch()
returns early without touching the DB when there's no q, Killswitch::isEnabled() is pure
constants, BlogRssFetcher goes through cache.app whose adapter swallows backend errors, and
the layout only touches the DB via app.user. So an anonymous / can still render during an
outage. (It is not a full DB-independence claim: /?q=<name> still does a findOneBy and
logged-in requests still load the user, so both will 500 either way.)
Important: the controller change needs the template change with it. Twig's number_format
does number_format((float) $number, ...), so an unguarded 'N/A' renders as 0 — "0
packages published" during an outage is worse than "N/A". search_section.html.twig already
guards downloads that way; packages and versions need the same. Both are inline
suggestions.
Two related things that pre-date this PR, so entirely your call whether to fold them in:
statsAction has the same narrow ConnectionException catch, and statsTotalsAction
(/statistics.json) does all three lookups with no error handling whatsoever.
2. clearfix does not break a Bootstrap 5 grid row
From the vendored 5.3.8 build:
.row{--bs-gutter-x:1.5rem;--bs-gutter-y:0;display:flex;flex-wrap:wrap;...}
.clearfix::after{display:block;clear:both;content:""}
clear: both is inert inside a flex container. The BS5 idiom for a grid line break is
<div class="w-100"></div>.
Concrete consequence in the section this PR edits: "How to submit packages?" now has five
col-lg-6 children (naming, creating-composer.json, validate-and-publish,
managing-package-versions, update-schedule). With the intended break, "Validate and publish"
ends line 2. Without it the flow is [naming | creating], [validate | managing-versions],
[update-schedule alone], leaving "Update Schedule" as an orphaned half-width column. Before
this PR the section had four children and laid out as two clean rows.
The second new one (just above "How to submit packages?") sits outside any .row — the
enclosing <section class="row"> closes on the line above it — so it can never do anything
either way. Both are inline suggestions.
Heads up that four pre-existing clearfix uses in the same file are silently broken for the
same reason. Not yours to fix, but if you're touching the file anyway it's a cheap sweep.
3. tests/Controller/PackageControllerTest.php looks like it wandered in from another branch
It has nothing to do with a homepage redesign — I'd revert it out of this PR. Separately, the
new comment is wrong: it says the payload is encoded with
JSON_UNESCAPED_SLASHES|JSON_UNESCAPED_UNICODE, but the assertion passes only
DEFAULT_ENCODING_OPTIONS | JSON_UNESCAPED_SLASHES (79) where production uses 335. It passes
today only because the fixture is pure ASCII, so the previous comment was vague and the new one
is actively incorrect. Fixing it properly means matching the flags and adding a non-ASCII
value so the difference can actually fail — better as its own PR.
Decisions for @Seldaek rather than changes to make here
The homepage can now render a completely empty content area. The content block is only the
news list, wrapped in {% if newsItems is defined and newsItems|length > 0 %}.
BlogRssFetcher::getNewsItems() returns [] on any exception, and additionally filters to
items categorised composer or packagist.org — so "zero items" is a realistic steady state,
not just a hard-failure state. In that case the visitor gets the hero followed by an empty white
.container.content. That failure mode didn't exist when the page had two columns of static
copy. Either a small fallback panel (popular/new packages) or skipping the .content wrapper
entirely would cover it.
SEO / content. The most-linked page on the site loses ~60 lines of keyword-rich copy
("Define Your Dependencies", "Install Composer", "Autoload Dependencies", "Publishing
Packages") and it lands on /about, which has far fewer inbound links. Legitimate product call,
just worth making deliberately rather than as a side-effect of a UI PR.
One content regression inside that move: the homepage's minimal publishing example (name /
description / require with "php": ">=8.2", explicitly labelled "the strictly minimal
information you have to give") is deleted with no replacement. The example that survives on
/about is the full Monolog one, which recommends psr-0 autoloading and
"php": ">=8.0.0". So "don't duplicate the composer.json example" ends up keeping the verbose,
stale one and dropping the concise, current one. Either keep the minimal snippet or refresh the
Monolog one to psr-4.
Homepage and /statistics will disagree. statsAction renders
'packages' => max($chart['packages']) from getCountByYearMonth() (3600s TTL) while the hero
uses getTotal() (86400s). Different queries, different caches, so the two pages show different
numbers. A single getTotals() helper would fix the drift and also de-triplicate the three
lookups now duplicated across index, statsAction and statsTotalsAction.
Dev-environment cost of the counts. result_cache_driver is configured only under
when@prod, so in dev/test doctrine-bundle falls back to an in-memory ArrayAdapter and
every dev homepage load runs SELECT COUNT(*) FROM package_version GROUP BY 1=1 against the
full table. Not a production problem, but noticeable while working on the page.
Smaller UX / a11y notes
- Duplicated brand identity. The hero renders
brandname(52px) +navclaim(20px italic),
and the sticky header directly above already shows
<h1 class="navbar-brand">Packagist <em class="d-none d-lg-inline">The PHP Package Repository</em></h1>.
Atlg+ both strings are visible about 60px apart. Also, the visually dominant text on the
page is adivwhile the realh1is the 24px nav brand — either promote the hero title to
h1and hide the nav duplicate on/, or mark the hero copyaria-hidden="true". - The hero isn't collapsed on an active search.
js/search.jsonly toggles#search-container's
d-none, so on/?q=foothe full hero (~300px) stays above the results and pushes the first
hit below the fold — with a 720px-wide search field sitting above ~1140px-wide results. Adding
a class to.wrapper-search-herothat hides.hero-search-intro/.hero-search-footer
while a query is active would handle it. - Header nav will probably overflow at
md.menu.about_packagistis "About Packagist.org"
(19 chars) and each nav link carriespadding: 18px 25px 19px— 50px per item. Logged out at
768px: brand (~130px) + Browse / About Packagist.org / Submit / Create account / Sign in
≈ 620px ≈ 750px against ~744px of usable container width, and logged-in is worse since
.username-linkis allowed up to 200px below 992px. Cheapest fix is a shortmenu.about
("About") key for the header, keeping the long form in the footer — or bump to
navbar-expand-lg. - Images. Measured: the new
logo-packagist.webpis 400×455 / 20,654 B, while the reused
logo-composer.pngis 290×356 / 104,378 B — 5× heavier than the logo this PR set out to
optimise, rendered at ~52×64, and now unconditional in the hero (it previously carried
d-none d-lg-inline-block). Neither<img>declareswidth/height, so the browser can't
reserve space before decode → CLS on our most-visited page (inline suggestions add the
intrinsic dimensions). Also, two different Packagist logos are now live: the sharp webp on/
and the 100×100logo-small.pngon every other page. The "current one is blurry" motivation
applies site-wide, so this leaves the inconsistency half-fixed. And there's no<picture>/PNG
fallback for the webp. - Hardcoded English. "packages published", "versions", "installs", "Install Composer",
"Discover Packagist" are literals, while the rest of the file and the layout consistently use
|trans.translations/messages.en.ymlalready shipsstatistics.registered("Packages
registered"),statistics.versions_avail("Versions available") andstatistics.installed
("Packages installed") — the wording differs from yours, so pick whichever you prefer, but the
strings should be translatable. Related:/statisticslabels installs with(since 2012-04-13); the bare hero total labelled "installs" reads like a current rate. - Hardcoded hover colour.
color: #d97c10—CLAUDE.mdasks for customisation via:root
overrides, and the palette already pairs--color-X/--color-X-dark
(--color-danger/--color-danger-dark, etc.). Adding--color-brand-orange-darkwould fit
the convention. Not left as a suggestion since it needs the:rootblock too. js-prefix carrying presentation.js-search-field-wrapperhas exactly one occurrence in
the codebase (the template itself) — no JS reads it. Styling off ajs-hook is a bit of a
trap for whoever greps for it later; a dedicated.hero-search-fieldclass would be clearer.
Verified clean, so you know what's already been checked
- CSP allows the new
.webp—img-src 'self' https: data:, and assets use a plain?v=
version strategy with no manifest lookup that could fail at runtime. |rawon the stat numbers is safe — the values areintfromCOUNT(*), and the pattern
is copied verbatim from the existingtemplates/web/stats.html.twig.- The
?? 0additions are correct and necessary —GROUP BY 1=1returns zero rows on an
empty table, so the old$result[0]['count']raisedUndefined array key 0, which is exactly
why the homepage test needed it. One thing to confirm: we run PHPStan level 8 with
reportUnmatchedIgnoredErrors: trueand there's no baseline entry for eithergetTotal(), so
the??may tripnullCoalesce.offset("always exists and is not nullable"). CI will tell
you. menu.about_packagistandsearch.claim_htmlboth exist, and the updated test's
'Packagist is the main'matches the translation.{% set showSearchDesc %}after{% extends %}matches the established pattern in 14 other
templates.- All existing
#how-to-*anchors referenced fromversion_list.html.twig/
view_package.html.twigsurvive the about-page move,#getting-startedwas correctly added to
the CSS, all about-pageids are unique,.publishing-packagesis correctly deleted now that
it's dead, andweb/build/is gitignored so no compiled CSS is expected in the diff.
Nothing here is a blocker beyond the three items in the first section — thanks for taking this
on, the search-first framing is the right call.
ac6858a to
fed3fe1
Compare
|
Apologies for the weird changes that were in the original PR, I guess agentic coding is not always so great 😢 Thanks for the deep review — replied inline on each item. Rebased onto current FixedAll "should fix" and inline-suggestion items are in: the Redis/DB error handling, the two broken Two more, not flagged as committable suggestions but raised in the writeup:
Left to you, as flagged
|
Seldaek
left a comment
There was a problem hiding this comment.
Round 2 — reviewed fed3fe19
Almost everything from the first pass landed, and CI is green across the board including PHPStan
— so the ?? 0 concern I flagged there is closed. Two of the fixes went further than suggested:
the h1 duplication is solved properly (hero title becomes the h1, nav brand switches to a
div on home), and the Monolog example got refreshed to psr-4 / "php": ">=8.2", which
wasn't asked for. Thanks for the thorough pass.
Two new findings, both fallout from changes I suggested — see the inline comments:
.hero-brand-titlepicks upmargin-top: 20px/margin-bottom: 10pxfrom theh1element
selector now that it is anh1. My suggestion, my miss — one-click fix inline.- The hero-collapse fix doesn't cover the initial page load, because
onStateChangeonly fires
on state changes. That's the reason the separate init block atjs/search.js:68-77exists.
One layout call worth a conscious look rather than a fix: the w-100 works now, and since the
"How to submit packages?" section has five col-lg-6 children one of them is always alone. The
break lands between "Validate and publish" and "Managing package versions", giving
[naming | creating], [validate alone], [managing | schedule] — a half-empty row mid-section
rather than at the end. Reads as deliberate grouping (validate closes the "create your package"
flow), so probably what you wanted, just flagging it's a choice.
Open points — @Seldaek to triage, not blocking this PR
Recording these so they don't get lost. None of them are asks of @EdouardCourty — they're
either pre-existing, or product calls, or follow-up work.
Product / content decisions
-
/can render a completely empty content area. Thecontentblock is only the news
list, andBlogRssFetcher::getNewsItems()returns[]on any exception and filters to
items categorisedcomposer/packagist.org— so "zero items" is a realistic steady state,
not just a hard-failure state. The visitor then gets the hero followed by an empty white
.container.content. That failure mode didn't exist when the page had two columns of static
copy. A small fallback panel (popular/new packages) or skipping the.contentwrapper would
cover it. This is the one I'd care about most. - SEO. The most-linked page on the site loses its indexable copy to
/about. Deliberate
call to confirm, now that the content itself is in good shape there. - "installs" has no qualifier.
/statisticslabels the same number
statistics.installed+(since 2012-04-13); a bare lifetime total labelled "installs" reads
like a current rate.
Consistency / correctness follow-ups
-
/and/statisticscompute totals differently —getTotal()(86400s cache) vs
max($chart['packages'])fromgetCountByYearMonth()(3600s). Different queries, different
TTLs, so the two pages will show different numbers. A sharedgetTotals()helper would fix the
drift and de-triplicate the lookups now duplicated acrossindex,statsActionand
statsTotalsAction. -
statsTotalsAction(/statistics.json) has no error handling at all, and
statsActionstill uses the narrowPredis\Connection\ConnectionException— so both still
500 on a-LOADING/-OOM/-READONLYreply, which is what this PR fixed for/. Same
widening applies. (Broader sweep of that pattern is #1829.) - Five inert
clearfixdivs remain inabout.html.twig(lines 7, 143, 172, 183, 196).
Same root cause as the one fixed here —clear: bothdoes nothing in a flex.row. Worth
noting the "How to update packages?" row has sevencol-lg-6children with three of those
clearfixes, so its intended grouping doesn't work either. Mechanicalw-100swap. - No test asserts the new markup.
testHomepagecoversassertResponseIsSuccessfuland
the claim text; nothing covers the three stat items, the number formatting, or the'N/A'
fallback path. Something likeassertCount(3, $crawler->filter('.hero-search-stats li'))would
lock the feature in. (The?? 0fix is implicitly covered —testHomepagedoesn't call
initializePackages(), so it already exercises the empty-table path.)
Assets / polish
-
logo-composer.pngis 104,378 B — 5× the new 20,654 B webp, 290×356 rendered at ~52×64,
and now unconditional in the hero where it previously carriedd-none d-lg-inline-block. It's
the heavy asset on the page this PR set out to lighten. - Two competing Packagist logos are live — the sharp webp on
/, the 100×100
logo-small.pngeverywhere else. The "current one is blurry" motivation applies site-wide, so
this is half-fixed. Also no<picture>/PNG fallback for the webp. -
js-search-field-wrapperis still the styling hook. The deadfloat: noneis gone as
suggested, but presentation still hangs off ajs-prefixed class that no JS reads (one
occurrence in the whole codebase, the template itself). A.hero-search-fieldclass would be
clearer — needs a template change, so it wasn't worth bundling here. - Mobile hero padding is unchanged. The
@media (max-width: 575px)block shrinks the
logos and title but leavespadding: 48px 20px 40pxon.wrapper-search-hero .container. - Cosmetic:
.hero-search-intro .hero-search-claim { margin: 0 auto }(specificity 0-2-0)
losesmargin-top/margin-bottomto.wrapper-search .container p(0-2-1), so the effective
margin-topis 8px, not 0. Harmless, but themargin: 0 autoreads as if it takes effect.
fed3fe1 to
134b5d7
Compare
134b5d7 to
d20b1a2
Compare

Closes #1822
What changed
The homepage was structured like a documentation page: two dense text columns
("Getting Started" / "Publishing Packages") above the fold, no usage stats, and the
search bar with no particular emphasis. This PR turns it into a search-first hero,
closer to what other package registries (PyPI, RubyGems, crates.io) do:
installs) → two action buttons ("Install Composer" externally, "Discover Packagist"
to the about page)
/about, mergedinto its existing sections instead of duplicating the composer.json example
Screenshots
New homepage

Updated about page
