Skip to content

fix(transactions): update dialog content class for new transaction view#1693

Merged
jjmata merged 5 commits intowe-promise:mainfrom
thomasbaker9010251:fix/layout
May 9, 2026
Merged

fix(transactions): update dialog content class for new transaction view#1693
jjmata merged 5 commits intowe-promise:mainfrom
thomasbaker9010251:fix/layout

Conversation

@thomasbaker9010251
Copy link
Copy Markdown
Contributor

@thomasbaker9010251 thomasbaker9010251 commented May 7, 2026

Screenshots

Before

Snipaste_2026-05-06_19-30-55

After

image

Summary by CodeRabbit

  • Bug Fixes
    • Improved scrolling behavior for dialog boxes on larger screens in the transaction creation flow.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 7, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e4133b65-33a8-4397-9ab4-310350a493b2

📥 Commits

Reviewing files that changed from the base of the PR and between 406f3ed and 96b4a1f.

📒 Files selected for processing (1)
  • app/views/transactions/new.html.erb
✅ Files skipped from review due to trivial changes (1)
  • app/views/transactions/new.html.erb

📝 Walkthrough

Walkthrough

A single-line update to app/views/transactions/new.html.erb adds Tailwind CSS classes (lg:max-h-none lg:overflow-y-auto) to the dialog content layer, controlling height and scrollbar behavior on large screens.

Changes

Dialog Content Styling

Layer / File(s) Summary
UI Rendering
app/views/transactions/new.html.erb
DS::Dialog now includes content_class with responsive Tailwind utilities to prevent max-height constraints and enable scrolling on large viewports, while keeping scrollable: false on the dialog component itself.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Possibly related PRs

  • we-promise/sure#1196: Modifies dialog rendering in the same file to adjust scroll/overflow behavior.
  • we-promise/sure#653: Updates DS dialog UI component implementation with safe-area padding adjustments.
  • we-promise/sure#739: Added support for the content_class attribute on DS::Dialog, enabling this usage.

Suggested labels

mobile/PWA

Suggested reviewers

  • alessiocappa
  • jjmata

Poem

🐰 With whiskers twitched and paws on keys,
A dialog blooms, responsive and free,
Max-height? Gone! On screens so wide,
The content scrolls with graceful pride,
Tailwind spins its magic thread!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and specifically describes the main change: updating a dialog content class in the new transaction view to fix layout issues.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@brin-security-scanner brin-security-scanner Bot added contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis. labels May 7, 2026
@thomasbaker9010251
Copy link
Copy Markdown
Contributor Author

@coderabbitai pls review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 7, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the changed file
fd "new.html.erb" --type f | grep -i transaction | xargs cat

# Also look for the DS::Dialog component to understand content_class
rg -n "content_class" --type erb -C3
rg -n "DS::Dialog" --type erb -C3 | head -60

Length of output: 8375


🏁 Script executed:

#!/bin/bash
# Find the DS::Dialog component
fd "dialog" --type f | grep -i "ds\|design_system\|component" | head -20

# Search for the Dialog component class definition
rg -n "class Dialog" --type rb -C5 | head -40

# Search for content_class and scrollable in the dialog component
rg -n "content_class\|scrollable" --type rb -C3 | head -60

Length of output: 208


🏁 Script executed:

#!/bin/bash
cat app/components/DS/dialog.rb
echo "---ERB---"
cat app/components/DS/dialog.html.erb

Length of output: 5364


@thomasbaker9010251 Here is my review of PR #1693.


Review: fix(transactions): update dialog content class for new transaction view

What the change does

In app/views/transactions/new.html.erb, content_class: "lg:max-h-none" is added to the DS::Dialog call:

<%= render DS::Dialog.new(scrollable: false, content_class: "lg:max-h-none") do |dialog| %>

Looking at app/components/DS/dialog.rb, content_class is appended at the end of dialog_inner_classes:

def dialog_inner_classes
  # ...modal case:
  class_names(
    "flex flex-col bg-container rounded-xl shadow-border-xs mx-3 lg:mx-0 w-full",
    "max-h-full",          # default
    WIDTHS[width],         # e.g. "lg:max-w-[550px]"
    content_class          # ← "lg:max-h-none" overrides max-h-full on lg+
  )
end

So on lg+ screens, lg:max-h-none overrides the default max-h-full, letting the dialog inner container grow to its natural content height rather than being capped at the viewport height. The before/after screenshots confirm this resolves the visual layout issue.


✅ What's correct

  • The root cause is clear: scrollable: false disables overflow-auto on the inner content div, so when max-h-full was capping the container at viewport height, content at the bottom (Notes textarea, "Add transaction" button) could be cut off on large screens.
  • lg:max-h-none is appropriately scoped to large screens only — mobile behavior (max-h-full) is unchanged.
  • Consistent with how content_class is already used elsewhere (e.g., _cashflow_sankey.html.erb uses content_class: "!w-[96vw] max-w-[1650px]").

⚠️ One concern — missing scroll fallback on large screens

With scrollable: false, the inner content <div> has no overflow-auto. With lg:max-h-none, there is no height cap. The outer <dialog> element is h-full (full viewport height). If the form content ever grows taller than the viewport (e.g., future fields added, or a very long category/merchant dropdown), the inner div will overflow the <dialog> container with no scroll mechanism, potentially clipping content.

Suggested safer alternative — use lg:max-h-[90dvh] with overflow-y-auto as a safety net, or combine lg:max-h-none with lg:overflow-y-auto in content_class:

<%= render DS::Dialog.new(scrollable: false, content_class: "lg:max-h-none lg:overflow-y-auto") do |dialog| %>

Or if the dialog itself handles vertical scrolling at the <dialog> element level for overflowing content (browser native), this may be acceptable as-is. Worth verifying this edge case in a browser with reduced viewport height.


Summary

Correctness ✅ Fixes the described layout issue
Scope ✅ Minimal, targeted change
Edge case ⚠️ No scroll fallback if form grows taller than viewport on lg+
Screenshots ✅ Before/after confirm the fix

The change is acceptable for the current form size. The scroll fallback concern is low risk today but worth noting for future-proofing.

petermilord and others added 3 commits May 7, 2026 06:09
… to prevent past dates

- Implemented a validation method to ensure the expiration date of credit cards is not in the past.
- Updated the credit card form to set a minimum date for the expiration date field, preventing users from selecting past dates.
- Changed the error message assertion for the expiration date validation to check for the symbol :greater_than_or_equal_to instead of a specific date string. This improves the flexibility and clarity of the validation error handling.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
app/views/credit_cards/_form.html.erb (1)

29-31: ⚠️ Potential issue | 🟠 Major

min: Date.current has the same day-granularity issue as the model validation.

This is the downstream effect of the concern noted in app/models/credit_card.rb: the HTML min attribute should mirror the model's boundary. If the fix there uses Date.current.beginning_of_month, apply the same here so the form and model stay consistent.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/views/credit_cards/_form.html.erb` around lines 29 - 31, The form's
expiration_date date_field uses min: Date.current which mismatches the model
boundary; update the date_field invocation (credit_card_form.date_field
:expiration_date) to use the same boundary as the model (e.g., min:
Date.current.beginning_of_month) so the HTML min attribute mirrors the model
validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@app/views/credit_cards/_form.html.erb`:
- Around line 29-31: The form's expiration_date date_field uses min:
Date.current which mismatches the model boundary; update the date_field
invocation (credit_card_form.date_field :expiration_date) to use the same
boundary as the model (e.g., min: Date.current.beginning_of_month) so the HTML
min attribute mirrors the model validation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d72f7df8-6596-4f04-9aad-23499828b285

📥 Commits

Reviewing files that changed from the base of the PR and between c963c87 and 406f3ed.

📒 Files selected for processing (3)
  • app/models/credit_card.rb
  • app/views/credit_cards/_form.html.erb
  • test/models/credit_card_test.rb
✅ Files skipped from review due to trivial changes (1)
  • test/models/credit_card_test.rb

Copy link
Copy Markdown
Collaborator

jjmata commented May 8, 2026

Nice, clean fix. The CodeRabbit suggestion to pair lg:max-h-none with lg:overflow-y-auto was already incorporated — good call. The combination is correct: scrollable: false disables overflow on the inner content div, lg:max-h-none removes the height cap on large screens, and lg:overflow-y-auto provides a scroll fallback if future form additions ever overflow the dialog. Minimal and well-targeted.

Reviewed by Claude Code


Generated by Claude Code

@jjmata
Copy link
Copy Markdown
Collaborator

jjmata commented May 9, 2026

👋 Hello Gittensor contributor ... please swing by our Discord and read #annoucements for a request for you.

@jjmata jjmata merged commit 43e7e35 into we-promise:main May 9, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis.

Development

Successfully merging this pull request may close these issues.

4 participants