You said "I just want a button and pop up". I did that, but I also reflowed every table into stacked cards, which you rightly said looked awkward. That is reverted — the tables are proper tables with their header rows again.
This PR fixes what was left: the advice is a button and a pop-up, and the table stays readable.
What was actually wrong
The tables were being squeezed to fit the screen. With seven to nine columns on a 390px phone, that meant:
A gravity of 1.0... and a date of 202... — truncated to nothing
The beer name painted over the gravity reading, because the cell was allowed to overflow its neighbour
Headings cut to R..., GRA..., BREWIN...
The "Open" button pushed off the right edge, reachable only by scrolling the table sideways
What it does now
The tables keep their natural width and scroll sideways inside their card. That is what a table does. Every value is fully readable:
20 | out on the patio | 1.039 39 points | — | Just doing a quick read | ✎ Advice | Open
The advice is a small button so the column that matters is narrow — the full text is in the pop-up, as you asked.
The headings have a short form for a phone.Brewing advice → Advice, Attenuation → Att., Overstated by → Over. The long form is still there on a desktop.
The secondary columns go away on a phone — the drop, the legacy ABV, the days. The details are still on the run's own page.
A name and a comment get a sensible minimum width and wrap on a word boundary, not one word per line.
Verified
Browser-measured at 390px and at 1280px, on every page:
No page overflows — /brews, /active, /history, /analysis and /runs/<id> all have scrollWidth == innerWidth.
Every value readable — the name, the gravity, the comment, the date and the ABV are all complete, not truncated.
No text overlaps another column — measured cell-by-cell across every row of every table.
The desktop is untouched — the same full headings (EST. ABV, BREWING ADVICE, FINISHED), the same 806px table, and the pop-out still opens on hover.
190 tests pass.
Test plan
190 tests pass
A regression test that the long heading is still in the page and the short form is available, and that the beer name is not truncated in the markup
Browser-measured at 390px and 1280px on every page
Cell-by-cell overlap check across every row of every table
Follow-up to #16 and #17.
## Why
You said *"I just want a button and pop up"*. I did that, but I also reflowed every table into stacked cards, which you rightly said looked awkward. That is reverted — the tables are proper tables with their header rows again.
This PR fixes what was left: the advice is a button and a pop-up, and the table stays readable.
## What was actually wrong
The tables were being squeezed to fit the screen. With seven to nine columns on a 390px phone, that meant:
- A gravity of `1.0...` and a date of `202...` — truncated to nothing
- The beer name **painted over the gravity reading**, because the cell was allowed to overflow its neighbour
- Headings cut to `R...`, `GRA...`, `BREWIN...`
- The "Open" button pushed off the right edge, reachable only by scrolling the table sideways
## What it does now
**The tables keep their natural width and scroll sideways inside their card.** That is what a table does. Every value is fully readable:
```
20 | out on the patio | 1.039 39 points | — | Just doing a quick read | ✎ Advice | Open
```
**The advice is a small button** so the column that matters is narrow — the full text is in the pop-up, as you asked.
**The headings have a short form for a phone.** `Brewing advice` → `Advice`, `Attenuation` → `Att.`, `Overstated by` → `Over`. The long form is still there on a desktop.
**The secondary columns go away on a phone** — the drop, the legacy ABV, the days. The details are still on the run's own page.
**A name and a comment get a sensible minimum width** and wrap on a word boundary, not one word per line.
## Verified
Browser-measured at 390px and at 1280px, on every page:
- **No page overflows** — `/brews`, `/active`, `/history`, `/analysis` and `/runs/<id>` all have `scrollWidth == innerWidth`.
- **Every value readable** — the name, the gravity, the comment, the date and the ABV are all complete, not truncated.
- **No text overlaps another column** — measured cell-by-cell across every row of every table.
- **The desktop is untouched** — the same full headings (`EST. ABV`, `BREWING ADVICE`, `FINISHED`), the same 806px table, and the pop-out still opens on hover.
190 tests pass.
## Test plan
- [x] 190 tests pass
- [x] A regression test that the long heading is still in the page and the short form is available, and that the beer name is not truncated in the markup
- [x] Browser-measured at 390px and 1280px on every page
- [x] Cell-by-cell overlap check across every row of every table
The tables were squeezed to fit the screen, and with seven to nine
columns on a 390px phone every heading and every value was truncated to
a few characters - a gravity of "1.0..." and a date of "202..." are
worse than useless, and the beer name was painted over the gravity
reading because the cell was allowed to overflow.
The tables keep their natural width and scroll sideways inside their
card, which is what a table does. The advice is a small button so the
column that matters is narrow, and the headings have a short form for
a phone ("Brewing advice" -> "Advice", "Attenuation" -> "Att.").
The secondary columns - the drop, the legacy ABV and the days - go away
on a phone; the details are still on the run's own page.
A name and a comment get a sensible minimum width and wrap on a word
boundary rather than one word per line.
The desktop is untouched: the same full headings, the same 806px table,
and the pop-out still opens on hover.
Synthesized Code Review Report: fix(ui): keep the table columns readable on a phone
1. Executive Summary
This Pull Request focuses on resolving critical UI/UX regressions on mobile devices (specifically 390px widths) where data tables were becoming unreadable. The developer has shifted from a "stacked card" approach to a "natural width" horizontal-scrolling approach, implemented responsive headers, and introduced conditional column visibility.
Across all three review domains (Code, Security, and Infrastructure), the consensus is highly positive. The changes are primarily confined to the presentation layer (CSS/HTML), resulting in a low-risk profile. The implementation is backed by strong verification rigor and new regression tests. While the technical approach is sound, there are minor recommendations regarding HTML maintainability and accessibility to ensure long-term code health.
2. Prioritized List of Issues
Priority
Issue
Domain
Description
Low
Potential HTML Redundancy
Code
The use of "short-form" and "long-form" headers may lead to duplicate HTML elements, increasing maintenance and potentially confusing screen readers.
Low
A11y for "Advice" Pop-up
Code
Transitioning static text to a button-triggered pop-up requires verification of keyboard navigation and ARIA attributes.
Low
CSS Maintainability
Code
Risk of "CSS bloat" if media queries are repeated across many classes rather than using utility classes.
Info
Visual Regression Gap
Infra
Current verification is manual/browser-measured; lacks automated visual regression testing.
3. Domain-Specific Recommendations
🛠 Code Quality & Maintainability
Refactor Header Logic: Instead of duplicating header elements for mobile/desktop, use a single element with a nested <span class="desktop-only"> or leverage CSS pseudo-elements/data-attributes to swap labels.
Implement CSS Variables: Define sensible minimum widths (e.g., --col-name-min-width: 150px) as variables in app.css to ensure consistency across the four affected templates.
Template DRY-ness: Evaluate the use of a Jinja2 macro for the "short/long" header logic to prevent duplication across analysis.html, brews.html, history.html, and runs/<id>.
Accessibility Audit: Ensure the new "Advice" button is keyboard-accessible (Tab/Enter) and includes aria-expanded state management.
🛡 Security
Maintain Current Posture: The PR is rated SECURE. No new attack vectors (XSS/Injection) were introduced as no new user-supplied data rendering methods were added.
General Recommendations:
Audit existing templates to ensure no | safe filters are used on user-controllable strings.
Verify that the "run's own page" (where hidden mobile data resides) maintains strict server-side authorization checks.
🏗 Infrastructure & DevOps
Build Optimization: Ensure the CI/CD pipeline is configured to minify app.css before production deployment to optimize mobile load times.
Testing Evolution: Consider integrating a visual regression tool (e.g., Playwright or Cypress) to replace manual browser measurements for future UI fixes.
4. Positive Highlights
UX Maturity: Correctly identified that horizontal scrolling is superior to stacked cards for technical data comparison.
Verification Excellence: Added specific regression tests for heading length and truncation, ensuring the fix is durable.
Efficient Refactoring: Successfully reduced the JavaScript footprint (app.js) while simultaneously improving the user experience.
5. Overall Assessment & Recommendation
Final Rating: $\text{Approved with Minor Changes}$
Justification:
The PR is technically sound, low-risk, and solves a significant user pain point. All reviewers agree that the security and infrastructure impacts are negligible. The "Minor Changes" recommendation is primarily focused on preventative maintenance (DRYing up the HTML/CSS) and accessibility to ensure the UI remains maintainable and inclusive.
Action Items for Developer:
Confirm the "short-form" headings do not create duplicate <th> tags that would confuse screen readers.
Verify keyboard accessibility for the new "Advice" pop-up.
(Optional but encouraged) Move repeated minimum widths to CSS variables.
## PR Review Results
# Synthesized Code Review Report: `fix(ui): keep the table columns readable on a phone`
## 1. Executive Summary
This Pull Request focuses on resolving critical UI/UX regressions on mobile devices (specifically 390px widths) where data tables were becoming unreadable. The developer has shifted from a "stacked card" approach to a "natural width" horizontal-scrolling approach, implemented responsive headers, and introduced conditional column visibility.
Across all three review domains (**Code, Security, and Infrastructure**), the consensus is highly positive. The changes are primarily confined to the presentation layer (CSS/HTML), resulting in a low-risk profile. The implementation is backed by strong verification rigor and new regression tests. While the technical approach is sound, there are minor recommendations regarding HTML maintainability and accessibility to ensure long-term code health.
---
## 2. Prioritized List of Issues
| Priority | Issue | Domain | Description |
| :--- | :--- | :--- | :--- |
| **Low** | **Potential HTML Redundancy** | Code | The use of "short-form" and "long-form" headers may lead to duplicate HTML elements, increasing maintenance and potentially confusing screen readers. |
| **Low** | **A11y for "Advice" Pop-up** | Code | Transitioning static text to a button-triggered pop-up requires verification of keyboard navigation and ARIA attributes. |
| **Low** | **CSS Maintainability** | Code | Risk of "CSS bloat" if media queries are repeated across many classes rather than using utility classes. |
| **Info** | **Visual Regression Gap** | Infra | Current verification is manual/browser-measured; lacks automated visual regression testing. |
---
## 3. Domain-Specific Recommendations
### 🛠 Code Quality & Maintainability
* **Refactor Header Logic:** Instead of duplicating header elements for mobile/desktop, use a single element with a nested `<span class="desktop-only">` or leverage CSS pseudo-elements/data-attributes to swap labels.
* **Implement CSS Variables:** Define sensible minimum widths (e.g., `--col-name-min-width: 150px`) as variables in `app.css` to ensure consistency across the four affected templates.
* **Template DRY-ness:** Evaluate the use of a Jinja2 macro for the "short/long" header logic to prevent duplication across `analysis.html`, `brews.html`, `history.html`, and `runs/<id>`.
* **Accessibility Audit:** Ensure the new "Advice" button is keyboard-accessible (Tab/Enter) and includes `aria-expanded` state management.
### 🛡 Security
* **Maintain Current Posture:** The PR is rated **SECURE**. No new attack vectors (XSS/Injection) were introduced as no new user-supplied data rendering methods were added.
* **General Recommendations:**
* Audit existing templates to ensure no `| safe` filters are used on user-controllable strings.
* Verify that the "run's own page" (where hidden mobile data resides) maintains strict server-side authorization checks.
### 🏗 Infrastructure & DevOps
* **Build Optimization:** Ensure the CI/CD pipeline is configured to minify `app.css` before production deployment to optimize mobile load times.
* **Testing Evolution:** Consider integrating a visual regression tool (e.g., Playwright or Cypress) to replace manual browser measurements for future UI fixes.
---
## 4. Positive Highlights
* **UX Maturity:** Correctly identified that horizontal scrolling is superior to stacked cards for technical data comparison.
* **Technical Rigor:** Implementation of explicit `scrollWidth == innerWidth` checks prevents layout jank.
* **Verification Excellence:** Added specific regression tests for heading length and truncation, ensuring the fix is durable.
* **Efficient Refactoring:** Successfully reduced the JavaScript footprint (`app.js`) while simultaneously improving the user experience.
---
## 5. Overall Assessment & Recommendation
**Final Rating: $\text{Approved with Minor Changes}$**
**Justification:**
The PR is technically sound, low-risk, and solves a significant user pain point. All reviewers agree that the security and infrastructure impacts are negligible. The "Minor Changes" recommendation is primarily focused on **preventative maintenance** (DRYing up the HTML/CSS) and **accessibility** to ensure the UI remains maintainable and inclusive.
**Action Items for Developer:**
1. Confirm the "short-form" headings do not create duplicate `<th>` tags that would confuse screen readers.
2. Verify keyboard accessibility for the new "Advice" pop-up.
3. (Optional but encouraged) Move repeated minimum widths to CSS variables.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Follow-up to #16 and #17.
Why
You said "I just want a button and pop up". I did that, but I also reflowed every table into stacked cards, which you rightly said looked awkward. That is reverted — the tables are proper tables with their header rows again.
This PR fixes what was left: the advice is a button and a pop-up, and the table stays readable.
What was actually wrong
The tables were being squeezed to fit the screen. With seven to nine columns on a 390px phone, that meant:
1.0...and a date of202...— truncated to nothingR...,GRA...,BREWIN...What it does now
The tables keep their natural width and scroll sideways inside their card. That is what a table does. Every value is fully readable:
The advice is a small button so the column that matters is narrow — the full text is in the pop-up, as you asked.
The headings have a short form for a phone.
Brewing advice→Advice,Attenuation→Att.,Overstated by→Over. The long form is still there on a desktop.The secondary columns go away on a phone — the drop, the legacy ABV, the days. The details are still on the run's own page.
A name and a comment get a sensible minimum width and wrap on a word boundary, not one word per line.
Verified
Browser-measured at 390px and at 1280px, on every page:
/brews,/active,/history,/analysisand/runs/<id>all havescrollWidth == innerWidth.EST. ABV,BREWING ADVICE,FINISHED), the same 806px table, and the pop-out still opens on hover.190 tests pass.
Test plan
The tables were squeezed to fit the screen, and with seven to nine columns on a 390px phone every heading and every value was truncated to a few characters - a gravity of "1.0..." and a date of "202..." are worse than useless, and the beer name was painted over the gravity reading because the cell was allowed to overflow. The tables keep their natural width and scroll sideways inside their card, which is what a table does. The advice is a small button so the column that matters is narrow, and the headings have a short form for a phone ("Brewing advice" -> "Advice", "Attenuation" -> "Att."). The secondary columns - the drop, the legacy ABV and the days - go away on a phone; the details are still on the run's own page. A name and a comment get a sensible minimum width and wrap on a word boundary rather than one word per line. The desktop is untouched: the same full headings, the same 806px table, and the pop-out still opens on hover.PR received — starting review, sit tight 🫡
PR Review Results
Synthesized Code Review Report:
fix(ui): keep the table columns readable on a phone1. Executive Summary
This Pull Request focuses on resolving critical UI/UX regressions on mobile devices (specifically 390px widths) where data tables were becoming unreadable. The developer has shifted from a "stacked card" approach to a "natural width" horizontal-scrolling approach, implemented responsive headers, and introduced conditional column visibility.
Across all three review domains (Code, Security, and Infrastructure), the consensus is highly positive. The changes are primarily confined to the presentation layer (CSS/HTML), resulting in a low-risk profile. The implementation is backed by strong verification rigor and new regression tests. While the technical approach is sound, there are minor recommendations regarding HTML maintainability and accessibility to ensure long-term code health.
2. Prioritized List of Issues
3. Domain-Specific Recommendations
🛠 Code Quality & Maintainability
<span class="desktop-only">or leverage CSS pseudo-elements/data-attributes to swap labels.--col-name-min-width: 150px) as variables inapp.cssto ensure consistency across the four affected templates.analysis.html,brews.html,history.html, andruns/<id>.aria-expandedstate management.🛡 Security
| safefilters are used on user-controllable strings.🏗 Infrastructure & DevOps
app.cssbefore production deployment to optimize mobile load times.4. Positive Highlights
scrollWidth == innerWidthchecks prevents layout jank.app.js) while simultaneously improving the user experience.5. Overall Assessment & Recommendation
Final Rating: $\text{Approved with Minor Changes}$
Justification:
The PR is technically sound, low-risk, and solves a significant user pain point. All reviewers agree that the security and infrastructure impacts are negligible. The "Minor Changes" recommendation is primarily focused on preventative maintenance (DRYing up the HTML/CSS) and accessibility to ensure the UI remains maintainable and inclusive.
Action Items for Developer:
<th>tags that would confuse screen readers.