ldraney/landscaping-assistant
1
0
Fork 0
Code Issues 32 Pull requests 1 Projects Releases Packages Wiki Activity Actions 4

Property detail page CSS refresh #177

Merged
ldraney merged 1 commit from 126-property-detail-css-refresh into main 2026-06-08 05:01:38 +00:00
Conversation 2 Commits 1 Files changed 3 +18 -24
ldraney commented 2026-06-08 04:52:28 +00:00
Owner
Copy link

Summary

  • Refreshes the property detail page layout so client name is the h1 heading and address moves to a subtitle line
  • Secondary action buttons get consistent outline styling in a new grid layout
  • Removes dead btn-link CSS class

Changes

  • app/views/properties/show.html.erb: h1 now renders client_name; address line + city/state/zip shown as subtitle; client name removed from definition list; "Go to Location" renamed to "Directions" with btn btn-outline; "Edit" upgraded from btn-link to btn btn-outline; secondary buttons wrapped in detail-actions-secondary div
  • app/assets/stylesheets/application.css: detail-actions switched from flex to grid; added detail-actions-secondary flex row; removed dead btn-link class
  • spec/requests/properties_spec.rb: updated show page spec to assert client_name renders as h1 heading

Test Plan

  • Tests pass locally
  • Visit a property detail page and confirm client name appears as h1 heading
  • Confirm address line appears as subtitle text below the heading
  • Confirm "Directions" and "Edit" render as outline-styled buttons in a row
  • No regressions in property list or edit pages

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive
  • Feature flag needed? No -- CSS refresh and layout change, no new user-visible workflow

Related Notes

  • Closes #126
  • landscaping-assistant -- landscaping assistant project
## Summary - Refreshes the property detail page layout so client name is the h1 heading and address moves to a subtitle line - Secondary action buttons get consistent outline styling in a new grid layout - Removes dead `btn-link` CSS class ## Changes - `app/views/properties/show.html.erb`: h1 now renders `client_name`; address line + city/state/zip shown as subtitle; client name removed from definition list; "Go to Location" renamed to "Directions" with `btn btn-outline`; "Edit" upgraded from `btn-link` to `btn btn-outline`; secondary buttons wrapped in `detail-actions-secondary` div - `app/assets/stylesheets/application.css`: `detail-actions` switched from flex to grid; added `detail-actions-secondary` flex row; removed dead `btn-link` class - `spec/requests/properties_spec.rb`: updated show page spec to assert client_name renders as h1 heading ## Test Plan - [ ] Tests pass locally - [ ] Visit a property detail page and confirm client name appears as h1 heading - [ ] Confirm address line appears as subtitle text below the heading - [ ] Confirm "Directions" and "Edit" render as outline-styled buttons in a row - [ ] No regressions in property list or edit pages ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive - [ ] Feature flag needed? No -- CSS refresh and layout change, no new user-visible workflow ## Related Notes - Closes #126 - `landscaping-assistant` -- landscaping assistant project
Property detail page CSS refresh (#126)
Some checks failed
ci/woodpecker/push/woodpecker Pipeline was successful
Details
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
CI / scan_ruby (pull_request) Has been cancelled
Details
CI / scan_js (pull_request) Has been cancelled
Details
CI / lint (pull_request) Has been cancelled
Details
9471401012 
- h1 now shows client_name; address line moves to subtitle paragraph
- Client name removed from definition list (no longer duplicated)
- "Go to Location" renamed to "Directions" with btn-outline style
- "Edit" link upgraded from btn-link to btn-outline
- Secondary buttons wrapped in detail-actions-secondary flex row
- detail-actions switched from flex to grid layout
- Removed dead btn-link class from CSS
- Updated spec to assert client_name renders as h1 heading

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney commented 2026-06-08 04:53:00 +00:00
Author
Owner
Copy link

QA Review -- PR #177

Scope: 3 files, +18/-24. CSS and view-only changes with spec update.

View (show.html.erb)

  • h1 correctly changed from address_line.presence || client_name to client_name -- client name is the stable identifier
  • Address line moves to subtitle paragraph with mdash separator -- clean formatting
  • "No address yet" fallback retained for properties without addresses
  • Client name removed from definition list -- eliminates duplication
  • "Go to Location" renamed to "Directions" -- concise, correct label
  • Both secondary buttons use btn btn-outline -- consistent styling
  • detail-actions-secondary wrapper provides clean structural grouping

CSS (application.css)

  • detail-actions switched from flex to grid with --spacing-sm gap -- uses existing design tokens
  • detail-actions-secondary as flex row with flex: 1 on child buttons -- equal-width, clean
  • Dead btn-link class removed -- no remaining references in codebase

Spec (properties_spec.rb)

  • Assertion tightened from include("Test Client") to include("<h1>Test Client</h1>") -- validates the structural change, not just content presence

Findings

No issues found. The diff is focused, uses existing design tokens, introduces no secrets or unrelated changes.

VERDICT: APPROVE

## QA Review -- PR #177 **Scope:** 3 files, +18/-24. CSS and view-only changes with spec update. ### View (`show.html.erb`) - h1 correctly changed from `address_line.presence || client_name` to `client_name` -- client name is the stable identifier - Address line moves to subtitle paragraph with mdash separator -- clean formatting - "No address yet" fallback retained for properties without addresses - Client name removed from definition list -- eliminates duplication - "Go to Location" renamed to "Directions" -- concise, correct label - Both secondary buttons use `btn btn-outline` -- consistent styling - `detail-actions-secondary` wrapper provides clean structural grouping ### CSS (`application.css`) - `detail-actions` switched from flex to grid with `--spacing-sm` gap -- uses existing design tokens - `detail-actions-secondary` as flex row with `flex: 1` on child buttons -- equal-width, clean - Dead `btn-link` class removed -- no remaining references in codebase ### Spec (`properties_spec.rb`) - Assertion tightened from `include("Test Client")` to `include("<h1>Test Client</h1>")` -- validates the structural change, not just content presence ### Findings No issues found. The diff is focused, uses existing design tokens, introduces no secrets or unrelated changes. **VERDICT: APPROVE**
ldraney commented 2026-06-08 04:59:41 +00:00
Author
Owner
Copy link

PR #177 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails (ERB views), vanilla CSS with design tokens, RSpec request specs.

ERB Correctness (show.html.erb)

  1. h1 change is safe. Property model has validates :client_name, presence: true (line 11 of app/models/property.rb), so @property.client_name will never be nil/blank. The old fallback (address_line.presence || client_name) is correctly removed.

  2. Subtitle line is correct. The address_line conditional on line 3-6 now renders the full address (address_line — city, state zip) when present, or "No address yet" otherwise. The &mdash; entity is valid HTML. All closing tags match.

  3. Client name removed from <dl> without regression. The <dt>Client</dt>/<dd> pair was redundant once client_name became the h1. No other view references the client row in the definition list.

  4. Secondary actions nesting is correct. The detail-actions-secondary div is properly nested inside detail-actions, and both link_to calls correctly use class: "btn btn-outline". The if @property.maps_url conditional is preserved. All ERB tags balance.

CSS Consistency (application.css)

  1. btn-outline exists at line 1123-1131. It uses design tokens: var(--color-surface), var(--color-accent), var(--color-accent-light). No hardcoded values. This class is already used in uploads/show.html.erb for the "Edit Caption" button, confirming it is the project's standard outline button pattern.

  2. detail-actions grid change is sound. Switching from display: flex to display: grid with gap: var(--spacing-sm) creates a single-column stack: the "Update Address" button on one row, the detail-actions-secondary flex row below it. This is a clean mobile-first layout.

  3. detail-actions-secondary uses design tokens only. display: flex, gap: var(--spacing-sm), and .btn { flex: 1 } -- all good. The flex: 1 ensures "Directions" and "Edit" split the row evenly.

  4. Dead btn-link removal is safe. I verified every view file: btn-link was ONLY used in show.html.erb (the two links being replaced). No other view, partial, or JS file references it. Clean removal.

Mobile Layout Analysis

The layout works on small screens. Key reasoning:

  • .app-shell constrains to max-width: 30rem with auto margins and padding: var(--spacing-md) -- already mobile-first.
  • .detail-actions is display: grid (single-column by default) so the "Update Address" button stacks above the secondary row.
  • .detail-actions-secondary is display: flex with flex: 1 on children -- "Directions" and "Edit" will share the row equally. On very narrow screens (under 375px), these will shrink proportionally since flex: 1 allows shrinking. At 375px minus padding (~343px usable), two buttons at ~170px each is comfortable.
  • The .btn base class has width: 100% and padding: var(--spacing-md) with font-size: 1.2rem -- good touch targets for mobile.

Regression Check

No regressions:

  • btn-link was only in show.html.erb -- confirmed by reading manage.html.erb, edit.html.erb, index.html.erb, uploads/show.html.erb, and profile/index.html.erb.
  • The old detail-actions .btn { width: auto; flex: none; } rule is correctly removed since detail-actions is now grid (not flex), and the children that need flex behavior are in detail-actions-secondary.
  • "Go to Location" renamed to "Directions" -- shorter label is better for mobile touch targets.

Test Coverage (spec/requests/properties_spec.rb)

The spec change at line 57-59 is appropriate:

  • Old: expect(response.body).to include("Test Client") -- too loose, would pass even if client_name appeared in a <dd> or elsewhere.
  • New: expect(response.body).to include("<h1>Test Client</h1>") -- verifies client_name renders as the page heading specifically.

The address assertion at line 53-54 (include("123 Test St")) still passes because the address now appears in the subtitle <p> tag.

BLOCKERS

None.

NITS

  1. Consider testing the subtitle format. The spec asserts "123 Test St" appears somewhere, but does not verify the new address_line — city, state zip format. A spec like expect(response.body).to include("123 Test St &mdash;") or matching the subtitle <p> would catch layout regressions. Non-blocking since the current test still validates address presence.

  2. detail-actions-secondary removed the old .detail-actions .btn rule which had width: auto; flex: none;. The "Update Address" button (direct child of .detail-actions, not inside detail-actions-secondary) now inherits .btn { width: 100% } from the base class, which is correct for grid layout (full-width primary action). Just noting this is intentional and correct.

SOP COMPLIANCE

  • Branch named after issue: 126-property-detail-css-refresh matches issue #126
  • PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
  • Related references the issue: Closes #126
  • No secrets committed: no .env files, credentials, or API keys in diff
  • No unnecessary file changes: exactly 3 files changed, all directly related to the CSS refresh
  • Commit messages: PR title is descriptive ("Property detail page CSS refresh")

PROCESS OBSERVATIONS

  • Small, focused PR (18 additions, 24 deletions across 3 files). Low change failure risk.
  • Net deletion of code (-6 lines) is a good sign -- removing dead CSS while improving layout.
  • The btn-link to btn-outline migration aligns with the existing pattern in uploads/show.html.erb, improving consistency across the app.

VERDICT: APPROVED

## PR #177 Review ### DOMAIN REVIEW **Tech stack:** Ruby on Rails (ERB views), vanilla CSS with design tokens, RSpec request specs. **ERB Correctness (show.html.erb)** 1. **h1 change is safe.** `Property` model has `validates :client_name, presence: true` (line 11 of `app/models/property.rb`), so `@property.client_name` will never be nil/blank. The old fallback (`address_line.presence || client_name`) is correctly removed. 2. **Subtitle line is correct.** The `address_line` conditional on line 3-6 now renders the full address (`address_line — city, state zip`) when present, or "No address yet" otherwise. The `&mdash;` entity is valid HTML. All closing tags match. 3. **Client name removed from `<dl>` without regression.** The `<dt>Client</dt>/<dd>` pair was redundant once `client_name` became the h1. No other view references the client row in the definition list. 4. **Secondary actions nesting is correct.** The `detail-actions-secondary` div is properly nested inside `detail-actions`, and both `link_to` calls correctly use `class: "btn btn-outline"`. The `if @property.maps_url` conditional is preserved. All ERB tags balance. **CSS Consistency (application.css)** 1. **`btn-outline` exists** at line 1123-1131. It uses design tokens: `var(--color-surface)`, `var(--color-accent)`, `var(--color-accent-light)`. No hardcoded values. This class is already used in `uploads/show.html.erb` for the "Edit Caption" button, confirming it is the project's standard outline button pattern. 2. **`detail-actions` grid change is sound.** Switching from `display: flex` to `display: grid` with `gap: var(--spacing-sm)` creates a single-column stack: the "Update Address" button on one row, the `detail-actions-secondary` flex row below it. This is a clean mobile-first layout. 3. **`detail-actions-secondary` uses design tokens only.** `display: flex`, `gap: var(--spacing-sm)`, and `.btn { flex: 1 }` -- all good. The `flex: 1` ensures "Directions" and "Edit" split the row evenly. 4. **Dead `btn-link` removal is safe.** I verified every view file: `btn-link` was ONLY used in `show.html.erb` (the two links being replaced). No other view, partial, or JS file references it. Clean removal. **Mobile Layout Analysis** The layout works on small screens. Key reasoning: - `.app-shell` constrains to `max-width: 30rem` with auto margins and `padding: var(--spacing-md)` -- already mobile-first. - `.detail-actions` is `display: grid` (single-column by default) so the "Update Address" button stacks above the secondary row. - `.detail-actions-secondary` is `display: flex` with `flex: 1` on children -- "Directions" and "Edit" will share the row equally. On very narrow screens (under 375px), these will shrink proportionally since `flex: 1` allows shrinking. At 375px minus padding (~343px usable), two buttons at ~170px each is comfortable. - The `.btn` base class has `width: 100%` and `padding: var(--spacing-md)` with `font-size: 1.2rem` -- good touch targets for mobile. **Regression Check** No regressions: - `btn-link` was only in `show.html.erb` -- confirmed by reading `manage.html.erb`, `edit.html.erb`, `index.html.erb`, `uploads/show.html.erb`, and `profile/index.html.erb`. - The old `detail-actions .btn { width: auto; flex: none; }` rule is correctly removed since `detail-actions` is now grid (not flex), and the children that need flex behavior are in `detail-actions-secondary`. - "Go to Location" renamed to "Directions" -- shorter label is better for mobile touch targets. **Test Coverage (spec/requests/properties_spec.rb)** The spec change at line 57-59 is appropriate: - Old: `expect(response.body).to include("Test Client")` -- too loose, would pass even if client_name appeared in a `<dd>` or elsewhere. - New: `expect(response.body).to include("<h1>Test Client</h1>")` -- verifies client_name renders as the page heading specifically. The address assertion at line 53-54 (`include("123 Test St")`) still passes because the address now appears in the subtitle `<p>` tag. ### BLOCKERS None. ### NITS 1. **Consider testing the subtitle format.** The spec asserts `"123 Test St"` appears somewhere, but does not verify the new `address_line — city, state zip` format. A spec like `expect(response.body).to include("123 Test St &mdash;")` or matching the subtitle `<p>` would catch layout regressions. Non-blocking since the current test still validates address presence. 2. **`detail-actions-secondary` removed the old `.detail-actions .btn` rule** which had `width: auto; flex: none;`. The "Update Address" button (direct child of `.detail-actions`, not inside `detail-actions-secondary`) now inherits `.btn { width: 100% }` from the base class, which is correct for grid layout (full-width primary action). Just noting this is intentional and correct. ### SOP COMPLIANCE - [x] Branch named after issue: `126-property-detail-css-refresh` matches issue #126 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references the issue: `Closes #126` - [x] No secrets committed: no .env files, credentials, or API keys in diff - [x] No unnecessary file changes: exactly 3 files changed, all directly related to the CSS refresh - [x] Commit messages: PR title is descriptive ("Property detail page CSS refresh") ### PROCESS OBSERVATIONS - Small, focused PR (18 additions, 24 deletions across 3 files). Low change failure risk. - Net deletion of code (-6 lines) is a good sign -- removing dead CSS while improving layout. - The btn-link to btn-outline migration aligns with the existing pattern in `uploads/show.html.erb`, improving consistency across the app. ### VERDICT: APPROVED
ldraney deleted branch 126-property-detail-css-refresh 2026-06-08 05:01:38 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
ldraney/landscaping-assistant!177
No description provided.