Repository navigation
fix(web): render a wide farm logo at its natural aspect in the sidebar - #498
Conversation
The last open piece of #179. .brand-logo was a fixed 26x26 square, and because it fits the whole image (object-fit: contain) a wide wordmark kept its aspect ratio and gave up its height — it rendered as a sliver. Contain prevents cropping; on a square slot it is exactly what collapses a wordmark. The slot is now height-driven (24px) with the width following the image's own aspect, bounded at 140px so a very wide mark cannot crowd the farm name out of the 244px sidebar. That bound only binds for a wide image: a compact mark stays ~24px and leaves .brand-name its full slot. This matches .logo-preview in Farm settings, which was already sized by height for the same reason. The farm name stays beside the logo (the issue's open question). A wordmark does then carry the name twice, but the alternative costs every farm with a compact mark its visible name, and the name slot already covers the no-logo and still-loading states. Retires the "upload a square mark" guidance across the Settings hint, the Help page, the in-app glossary (en/es/tl) and GLOSSARY.md: that was a workaround for the slot, not advice about logos, and #496's banner already gave detailed art a home. What the copy still rules out is a detailed picture, which is genuinely unreadable at sidebar size.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4d0487067
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const start = css.indexOf(`\n${selector} {`); | ||
| if (start === -1) throw new Error(`No top-level rule for '${selector}' in styles.css`); | ||
| const open = css.indexOf("{", start); | ||
| const close = css.indexOf("}", open); |
There was a problem hiding this comment.
If a later rule—such as a responsive override—reintroduces .brand-logo { width: 26px; height: 26px }, this helper still parses only the first matching block, so every assertion remains green while the CSS cascade renders the logo as a square again. Since this test is explicitly a regression guard for re-pinning the width, either resolve all applicable declarations or assert that no additional .brand-logo rule exists.
AGENTS.md reference: AGENTS.md:L180-L184
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real hole, fixed in 24f2ebd. The helper now collects every style rule mentioning .brand-logo at any nesting depth (at-rule preludes are descended into rather than treated as declarations), and the width assertion is quantified over all of them instead of the first match. Added a non-empty check too, so a renamed class fails rather than vacuously passing over an empty list.
Mutation-checked against a green baseline — all three turn it red, and baseline plus restore are green:
- your exact case,
@media (max-width: 1200px) { .brand-logo { width: 26px; height: 26px } }→ 1 failed - base rule reverted to
width: 26px→ 2 failed - class renamed away → 2 failed
Codex review of #498: the helper found the first `.brand-logo` block and stopped, so a later `@media { .brand-logo { width: 26px } }` would win the cascade and render the square again with this file still green — a guard that reads as safety while missing the thing it guards. Now collects every style rule mentioning the class at any nesting depth (including inside at-rules) and asserts no rule pins a fixed width, plus a non-empty check so a renamed class fails instead of vacuously passing over an empty list. Mutation-checked against a green baseline: a media-query override re-pinning the square, the base rule reverted to `width: 26px`, and the class renamed away each turn it red; baseline and restore both green.
|
@codex the first-match parsing hole is fixed in 24f2ebd — the guard is now quantified over every |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Closes #179 — the last open piece (the sidebar mark). Pieces 2 and 3 (post-login splash, separate hero asset) shipped in #496.
The defect
.brand-logowas a fixed26px × 26px. Withobject-fit: containa wide wordmark keeps its aspect ratio and gives up its height, so it rendered as a sliver.containprevents cropping — on a square slot it is precisely what collapses a wordmark, which is the subtlety the original #123 comment got backwards.The change
Height-driven (
24px), width follows the image's own aspect,max-width: 140px.The bound only binds for a wide image. A compact mark stays ~24px and leaves
.brand-nameits full slot; a wordmark takes the space instead, which is the right trade because a wordmark already carries the farm's name..logo-previewin Farm settings was already sized by height for the same reason — the sidebar was the outlier.The issue's open question: name vs. logo
Kept both the logo and the farm name. A wordmark does then show the name twice. The alternative (hide
.brand-name, move the name toalt) costs every farm with a compact mark its visible name, and the name slot already covers the no-logo and still-loading states. Recorded here because #179 asked for an explicit decision.Docs
The "upload a square mark" guidance is retired across the Settings hint, the Help page, the in-app glossary (en/es/tl) and
GLOSSARY.md. It was a workaround for the slot rather than advice about logos, and #496's banner already gave detailed art a home of its own. What the copy still rules out is a detailed picture — genuinely unreadable at sidebar size, and pointed at the banner instead.Testing
web/src/components/FarmBrand.styles.test.ts(new). Scope stated honestly in the file: jsdom does not lay out, so no test here can prove a wordmark renders correctly —getBoundingClientRectis all zeros. It reads the stylesheet and pins the declarations, catching exactly one regression: re-pinning.brand-logoto a fixed square. Real-browser behaviour was checked manually.expected '26px' to be 'auto', plus a missingmax-width. Green after the CSS change.SettingsPage.test.tsxhad the old guidance pinned (/Use a/+/square/). Rewritten to assert the surviving advice and the absence of the retired advice.FarmLogo.cs,FarmBannerOptions.cs,cluckwork.ts,styles.css.No migration, no API change, no new i18n keys (three existing strings reworded in all three catalogs).